Skip to content

Add parsing method for ElasticsearchException.generateThrowableXContent() #22783

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

Merged
merged 6 commits into from
Jan 26, 2017
Merged

Add parsing method for ElasticsearchException.generateThrowableXContent() #22783

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7c079f1
Move up fromXContent() method
tlrx Jan 25, 2017
5d32a94
Make fromXContent not fail on unknown array/object metadata
tlrx Jan 25, 2017
5ac4801
Update after Luca review
tlrx Jan 25, 2017
0ac62a8
Update after Luca review (2)
tlrx Jan 26, 2017
4d43094
Only parse string values, do not parse nulls
tlrx Jan 26, 2017
320023c
Only parse strings in metadata
tlrx Jan 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
149 changes: 82 additions & 67 deletions core/src/main/java/org/elasticsearch/ElasticsearchException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@
import static java.util.Collections.unmodifiableMap;
import static org.elasticsearch.cluster.metadata.IndexMetaData.INDEX_UUID_NA_VALUE;
import static org.elasticsearch.common.xcontent.XContentParserUtils.ensureExpectedToken;
import static org.elasticsearch.common.xcontent.XContentParserUtils.throwUnknownField;

/**
* A base class for all elasticsearch exceptions.
Expand Down Expand Up @@ -391,12 +390,93 @@ private static void headerToXContent(XContentBuilder builder, String key, List<S
protected void metadataToXContent(XContentBuilder builder, Params params) throws IOException {
}

/**
* Generate a {@link ElasticsearchException} from a {@link XContentParser}. This does not
* return the original exception type (ie NodeClosedException for example) but just wraps
* the type, the reason and the cause of the exception. It also recursively parses the
* tree structure of the cause, returning it as a tree structure of {@link ElasticsearchException}
* instances.
*/
public static ElasticsearchException fromXContent(XContentParser parser) throws IOException {
XContentParser.Token token = parser.nextToken();
ensureExpectedToken(XContentParser.Token.FIELD_NAME, token, parser::getTokenLocation);

return innerFromXContent(parser);
}

private static ElasticsearchException innerFromXContent(XContentParser parser) throws IOException {
String type = null, reason = null, stack = null;
ElasticsearchException cause = null;
Map<String, List<String>> metadata = new HashMap<>();
Map<String, Object> headers = new HashMap<>();

XContentParser.Token token = parser.currentToken();
Copy link
Member

@javanna javanna Jan 25, 2017

Choose a reason for hiding this comment

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

shall we check what the current token is? if it is a field_name we could use a while instead of a do while? that would be a bit more readable I think.

Copy link
Member Author

@tlrx tlrx Jan 25, 2017

Choose a reason for hiding this comment

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

Oh, I didn't address this point yet.

Copy link
Member

@javanna javanna Jan 25, 2017

Choose a reason for hiding this comment

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

same as before: shall we check what the current token is? if it is a field_name we could use a while instead of a do while? that would be a bit more readable I think.

Copy link
Member Author

@tlrx tlrx Jan 26, 2017

Choose a reason for hiding this comment

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

I changed this part a bit. It's tricky to use a while ((token = parser.nextToken()) == ...) like we do in other parsers because we mustn't parse the next token while entering the loop but we still need to do it while leaving the loop (so that the method left the parser in a coherent state, ie right after the last known field has been parsed).

I changed this to use a for loop, hoping it's easier to read now.

String currentFieldName = parser.currentName();
do {
if (token == XContentParser.Token.FIELD_NAME) {
currentFieldName = parser.currentName();
token = parser.nextToken();
}

if ( token != null && token.isValue()) {
Copy link
Member

@javanna javanna Jan 25, 2017

Choose a reason for hiding this comment

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

how can the token be null? that seems like a bug?

if (TYPE.equals(currentFieldName)) {
type = parser.text();
} else if (REASON.equals(currentFieldName)) {
reason = parser.text();
} else if (STACK_TRACE.equals(currentFieldName)) {
stack = parser.text();
} else {
metadata.put(currentFieldName, Collections.singletonList(parser.text()));
Copy link
Member

@javanna javanna Jan 25, 2017

Choose a reason for hiding this comment

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

I don't think we support arrays correctly here do we? metadata can be a list of values but here we create a new list for each value, and the last one wins?

Copy link
Member

@javanna javanna Jan 25, 2017

Choose a reason for hiding this comment

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

oh actually we skip all arrays in this parser, so I think we support only key-pairs, but we may print out arrays too?

Copy link
Member Author

@tlrx tlrx Jan 25, 2017

Choose a reason for hiding this comment

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

Metadata arrays are not supported at all and just skipped - we could decide to support arrays of value but this is out of scope of this pull request I think

Copy link
Member

@javanna javanna Jan 25, 2017

Choose a reason for hiding this comment

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

Maybe we should at least parse List given that we have that in some subclass? I wouldn't extend support for objects etc. here, but just for arrays of strings. that is what the addMetadata supports at the moment. I am not talking about supporting anything that may get printed when overriding metadataToXContent.

Copy link
Member Author

@tlrx tlrx Jan 25, 2017

Choose a reason for hiding this comment

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

You're right - we must support what gets printed and it can print arrays of values. I should have added that but I forgot it on my way... Thanks for raising this.

Copy link
Member

@javanna javanna Jan 26, 2017

Choose a reason for hiding this comment

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

shall we also skip non strings here?

Copy link
Member Author

@tlrx tlrx Jan 26, 2017

Choose a reason for hiding this comment

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

If we skip strings here, that would mean that some non-string metadata (like bytes_wanted & bytes_limit from CircuitBreakingException or line & col from ParsingException) won't be parsed back... which is sad I think, but maybe more coherent.

Copy link
Member

@javanna javanna Jan 26, 2017

Choose a reason for hiding this comment

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

yes I know, I value consistency here, let's not support things only in specific cases. we will parse those back when/if we will support proper numbers.

Copy link
Member Author

@tlrx tlrx Jan 26, 2017

Choose a reason for hiding this comment

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

Let's do that then

}
} else if (token == XContentParser.Token.START_OBJECT) {
if (CAUSED_BY.equals(currentFieldName)) {
cause = fromXContent(parser);
} else if (HEADER.equals(currentFieldName)) {
headers.putAll(parser.map());
Copy link
Member

@javanna javanna Jan 25, 2017

Choose a reason for hiding this comment

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

I think I would parse headers more precisely into a Map<String, List<String>>, it shouldn't be a lot of code.

Copy link
Member Author

@tlrx tlrx Jan 25, 2017

Choose a reason for hiding this comment

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

Make sense

} else {
// Additional metadata added by the metadataToXContent method is ignored
// and skipped, so that the parser does not fail on unknown fields.
parser.skipChildren();
}
}else if (token == XContentParser.Token.START_ARRAY) {
Copy link
Member

@javanna javanna Jan 25, 2017

Choose a reason for hiding this comment

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

can you add a whitespace after the bracket please?

// Additional metadata added by the metadataToXContent method is ignored
// and skipped, so that the parser does not fail on unknown fields.
Copy link
Member

@javanna javanna Jan 25, 2017

Choose a reason for hiding this comment

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

can we specify that only complex metadata (object and arrays) are not parsed? This will have to be updated though if we move subclasses to use addMetadata whenever possible, later.

Also, I wonder if we can do better. We could parse this unknown info into a map, but then there wouldn't be a place for it in the ElasticsearchException, unless we make metadata Map<String, Object>. Let's think about it, it may make sense for the future, not now maybe.

Copy link
Member Author

@tlrx tlrx Jan 25, 2017

Choose a reason for hiding this comment

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

I agree, we may or may not change metadata to be Map<String, Object> but I think we should address this later, out of the scope of this pull request. For now, let's just ensure that the parser does not fail.

Copy link
Member

@javanna javanna Jan 25, 2017

Choose a reason for hiding this comment

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

ok

parser.skipChildren();
}
} while ((token = parser.nextToken()) == XContentParser.Token.FIELD_NAME);

StringBuilder message = new StringBuilder("Elasticsearch exception [");
message.append(TYPE).append('=').append(type).append(", ");
message.append(REASON).append('=').append(reason);
if (stack != null) {
message.append(", ").append(STACK_TRACE).append('=').append(stack);
}
message.append(']');
Copy link
Member

@javanna javanna Jan 25, 2017

Choose a reason for hiding this comment

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

Can we compare messages from a normal ElasticsearchException with and without cause, with ones obtained from a parsed one? I wonder specifically if the Elasticsearch exception [ prefix is needed etc.

Copy link
Member Author

@tlrx tlrx Jan 25, 2017

Choose a reason for hiding this comment

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

I'm not sure to understand what you mean, but with or without cause you'll end up with something like Elasticsearch exception [type=foo, reason=bar], and with few cycles of toXContent/fromXContent it will end up with Elasticsearch exception [type=exception, reason=Elasticsearch exception [type=exception, reason=... etc]] which is not great.

Another bad point is the current "No ElasticsearchException found" in the generateFailureXContent() method which can obfuscate any non ElasticsearchException :(

Copy link
Member

@javanna javanna Jan 25, 2017

Choose a reason for hiding this comment

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

I was wondering what the difference is between an exception that is created not going through parsing, and an exception that gets parsed. Wondering about the message specifically.


ElasticsearchException e = new ElasticsearchException(message.toString(), cause);

for (Map.Entry<String, List<String>> entry : metadata.entrySet()) {
//subclasses can print out additional metadata through the metadataToXContent method. Simple key-value pairs will be
//parsed back and become part of this metadata set, while objects and arrays are not supported when parsing back.
//Those key-value pairs become part of the metadata set and inherit the "es." prefix as that is currently required
//by addMetadata. The prefix will get stripped out when printing metadata out so it will be effectively invisible.
//TODO move subclasses that print out simple metadata to using addMetadata directly and support also numbers and booleans.
//TODO rename metadataToXContent and have only SearchPhaseExecutionException use it, which prints out complex objects
e.addMetadata("es." + entry.getKey(), entry.getValue());
}
for (Map.Entry<String, Object> header : headers.entrySet()) {
e.addHeader(header.getKey(), String.valueOf(header.getValue()));
}
return e;
}

/**
* Static toXContent helper method that renders {@link org.elasticsearch.ElasticsearchException} or {@link Throwable} instances
* as XContent, delegating the rendering to {@link #toXContent(XContentBuilder, Params)}
* or {@link #innerToXContent(XContentBuilder, Params, Throwable, String, String, Map, Map, Throwable)}.
*
* This method is usually used when the {@link Throwable} is rendered as a part of another XContent object.
* This method is usually used when the {@link Throwable} is rendered as a part of another XContent object, and its result can
* be parsed back using the {@link #fromXContent(XContentParser)} method.
*/
public static void generateThrowableXContent(XContentBuilder builder, Params params, Throwable t) throws IOException {
t = ExceptionsHelper.unwrapCause(t);
Expand Down Expand Up @@ -455,71 +535,6 @@ public static void generateFailureXContent(XContentBuilder builder, Params param
builder.endObject();
}

/**
* Generate a {@link ElasticsearchException} from a {@link XContentParser}. This does not
* return the original exception type (ie NodeClosedException for example) but just wraps
* the type, the reason and the cause of the exception. It also recursively parses the
* tree structure of the cause, returning it as a tree structure of {@link ElasticsearchException}
* instances.
*/
public static ElasticsearchException fromXContent(XContentParser parser) throws IOException {
XContentParser.Token token = parser.nextToken();
ensureExpectedToken(XContentParser.Token.FIELD_NAME, token, parser::getTokenLocation);

String type = null, reason = null, stack = null;
ElasticsearchException cause = null;
Map<String, List<String>> metadata = new HashMap<>();
Map<String, Object> headers = new HashMap<>();

do {
String currentFieldName = parser.currentName();
token = parser.nextToken();
if (token.isValue()) {
if (TYPE.equals(currentFieldName)) {
type = parser.text();
} else if (REASON.equals(currentFieldName)) {
reason = parser.text();
} else if (STACK_TRACE.equals(currentFieldName)) {
stack = parser.text();
} else {
metadata.put(currentFieldName, Collections.singletonList(parser.text()));
}
} else if (token == XContentParser.Token.START_OBJECT) {
if (CAUSED_BY.equals(currentFieldName)) {
cause = fromXContent(parser);
} else if (HEADER.equals(currentFieldName)) {
headers.putAll(parser.map());
} else {
throwUnknownField(currentFieldName, parser.getTokenLocation());
}
}
} while ((token = parser.nextToken()) == XContentParser.Token.FIELD_NAME);

StringBuilder message = new StringBuilder("Elasticsearch exception [");
message.append(TYPE).append('=').append(type).append(", ");
message.append(REASON).append('=').append(reason);
if (stack != null) {
message.append(", ").append(STACK_TRACE).append('=').append(stack);
}
message.append(']');

ElasticsearchException e = new ElasticsearchException(message.toString(), cause);

for (Map.Entry<String, List<String>> entry : metadata.entrySet()) {
//subclasses can print out additional metadata through the metadataToXContent method. Simple key-value pairs will be
//parsed back and become part of this metadata set, while objects and arrays are not supported when parsing back.
//Those key-value pairs become part of the metadata set and inherit the "es." prefix as that is currently required
//by addMetadata. The prefix will get stripped out when printing metadata out so it will be effectively invisible.
//TODO move subclasses that print out simple metadata to using addMetadata directly and support also numbers and booleans.
//TODO rename metadataToXContent and have only SearchPhaseExecutionException use it, which prints out complex objects
e.addMetadata("es." + entry.getKey(), entry.getValue());
}
for (Map.Entry<String, Object> header : headers.entrySet()) {
e.addHeader(header.getKey(), String.valueOf(header.getValue()));
}
return e;
}

/**
* Returns the root cause of this exception or multiple if different shards caused different exceptions
*/
Expand Down
97 changes: 97 additions & 0 deletions core/src/test/java/org/elasticsearch/ElasticsearchExceptionTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,10 @@
import org.elasticsearch.cluster.block.ClusterBlockException;
import org.elasticsearch.common.ParsingException;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.breaker.CircuitBreakingException;
import org.elasticsearch.common.bytes.BytesArray;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.collect.Tuple;
import org.elasticsearch.common.xcontent.ToXContent;
import org.elasticsearch.common.xcontent.XContent;
import org.elasticsearch.common.xcontent.XContentBuilder;
Expand Down Expand Up @@ -516,6 +518,41 @@ public void testFromXContentWithHeadersAndMetadata() throws IOException {
assertThat(cause.getMetadata("es.index_uuid"), hasItem("_na_"));
}

public void testThrowableToAndFromXContent() throws IOException {
final XContent xContent = randomFrom(XContentType.values()).xContent();

final Tuple<Throwable, ElasticsearchException> exceptions = randomExceptions();
Copy link
Member

@javanna javanna Jan 25, 2017

Choose a reason for hiding this comment

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

can we test what happens when one calls setResources passing in multiple ids? That's the one place where we need lists to work properly.

final Throwable throwable = exceptions.v1();

BytesReference throwableBytes = XContentHelper.toXContent((b, p) -> {
ElasticsearchException.generateThrowableXContent(b, ToXContent.EMPTY_PARAMS, throwable);
Copy link
Member

@javanna javanna Jan 25, 2017

Choose a reason for hiding this comment

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

we may have done this somewhere else but I think for correct-ness we should rather pass p in instead of empty params. I would also love to call them builder and params if possible.

Copy link
Member Author

@tlrx tlrx Jan 25, 2017

Choose a reason for hiding this comment

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

Sure

return b;
}, xContent.type(), randomBoolean());

ElasticsearchException parsedException;
try (XContentParser parser = createParser(xContent, throwableBytes)) {
assertEquals(XContentParser.Token.START_OBJECT, parser.nextToken());
parsedException = ElasticsearchException.fromXContent(parser);
assertEquals(XContentParser.Token.END_OBJECT, parser.currentToken());
assertNull(parser.nextToken());
}
assertNotNull(parsedException);

ElasticsearchException expected = exceptions.v2();
while (expected != null) {
Copy link
Member

@javanna javanna Jan 25, 2017

Choose a reason for hiding this comment

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

nit: here I would maybe use do while just to make absolutely sure that the first round always runs.

assertEquals(expected.getMessage(), parsedException.getMessage());
assertEquals(expected.getHeaders(), parsedException.getHeaders());
assertEquals(expected.getMetadata(), parsedException.getMetadata());

if (expected.getCause() != null) {
expected = (ElasticsearchException) expected.getCause();
parsedException = (ElasticsearchException) parsedException.getCause();
} else {
assertNull(parsedException.getCause());
}
}
}

/**
* Builds a {@link ToXContent} using a JSON XContentBuilder and check the resulting string with the given {@link Matcher}.
*
Expand All @@ -533,4 +570,64 @@ private static void assertExceptionAsJson(Exception e, String expectedJson) thro
return builder;
}, expectedJson);
}

private static Tuple<Throwable, ElasticsearchException> randomExceptions() {
Throwable actual;
ElasticsearchException expected;

int type = randomIntBetween(0, 5);
switch (type) {
case 0:
actual = new ClusterBlockException(singleton(DiscoverySettings.NO_MASTER_BLOCK_WRITES));
Copy link
Member

@javanna javanna Jan 25, 2017

Choose a reason for hiding this comment

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

oh I see the trouble that I caused you, because not all of them are actually ElasticsearchException. Good test though, much more coverage now!

expected = new ElasticsearchException("Elasticsearch exception [type=cluster_block_exception, " +
"reason=blocked by: [SERVICE_UNAVAILABLE/2/no master];]");
break;
case 1:
actual = new CircuitBreakingException("Data too large", 123, 456);
expected = new ElasticsearchException("Elasticsearch exception [type=circuit_breaking_exception, reason=Data too large]");
expected.addMetadata("es.bytes_wanted", "123");
expected.addMetadata("es.bytes_limit", "456");
break;
case 2:
actual = new SearchParseException(new TestSearchContext(null), "Parse failure", new XContentLocation(12, 98));
expected = new ElasticsearchException("Elasticsearch exception [type=search_parse_exception, reason=Parse failure]");
expected.addMetadata("es.line", "12");
expected.addMetadata("es.col", "98");
break;
case 3:
actual = new IllegalArgumentException("Closed resource", new RuntimeException("Resource"));
expected = new ElasticsearchException("Elasticsearch exception [type=illegal_argument_exception, reason=Closed resource]",
new ElasticsearchException("Elasticsearch exception [type=runtime_exception, reason=Resource]"));
break;
case 4:
actual = new SearchPhaseExecutionException("search", "all shards failed",
new ShardSearchFailure[]{
new ShardSearchFailure(new ParsingException(1, 2, "foobar", null),
new SearchShardTarget("node_1", new Index("foo", "_na_"), 1))
});
expected = new ElasticsearchException("Elasticsearch exception [type=search_phase_execution_exception, " +
"reason=all shards failed]");
expected.addMetadata("es.grouped", "true");
expected.addMetadata("es.phase", "search");
break;
case 5:
actual = new ElasticsearchException("Parsing failed",
new ParsingException(9, 42, "Wrong state",
new NullPointerException("Unexpected null value")));
((ElasticsearchException) actual).addHeader("doc_id", "test");

ElasticsearchException expectedCause = new ElasticsearchException("Elasticsearch exception [type=parsing_exception, " +
"reason=Wrong state]", new ElasticsearchException("Elasticsearch exception [type=null_pointer_exception, " +
"reason=Unexpected null value]"));
expectedCause.addMetadata("es.line", "9");
expectedCause.addMetadata("es.col", "42");

expected = new ElasticsearchException("Elasticsearch exception [type=exception, reason=Parsing failed]", expectedCause);
expected.addHeader("doc_id", "test");
Copy link
Member

@javanna javanna Jan 25, 2017

Choose a reason for hiding this comment

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

should we rather add header or metadata outside of the switch to any generated exception (randomly)? Also test lists for headers too.

break;
default:
throw new IllegalArgumentException("No randomized exceptions generated for type [" + type + "]");
Copy link
Member

@javanna javanna Jan 25, 2017

Choose a reason for hiding this comment

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

nit: throw UnsupportedOperationException instead?

}
Copy link
Member

@javanna javanna Jan 26, 2017

Choose a reason for hiding this comment

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

can we test the case where we have numbers which get ignored?

Copy link
Member Author

@tlrx tlrx Jan 26, 2017

Choose a reason for hiding this comment

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

I added a dedicated test for this.

Copy link
Member

@javanna javanna Jan 26, 2017

Choose a reason for hiding this comment

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

wonderful, thanks

return new Tuple<>(actual, expected);
}
}
Loading