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

Add parsing method for ElasticsearchException.generateThrowableXContent() #22783

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 5 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
182 changes: 115 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 @@ -36,6 +36,7 @@
import org.elasticsearch.transport.TcpTransport;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
Expand All @@ -50,7 +51,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 +391,125 @@ 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 {
XContentParser.Token token = parser.currentToken();
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, List<String>> headers = new HashMap<>();

for (; token == XContentParser.Token.FIELD_NAME; token = parser.nextToken()) {
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()));
Copy link
Member

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

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

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

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

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

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

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

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

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)) {
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
if (token == XContentParser.Token.FIELD_NAME) {
currentFieldName = parser.currentName();
} else {
List<String> values = headers.getOrDefault(currentFieldName, new ArrayList<>());
if (token == XContentParser.Token.VALUE_STRING) {
values.add(parser.text());
} else if (token == XContentParser.Token.START_ARRAY) {
while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) {
if (token == XContentParser.Token.VALUE_STRING) {
values.add(parser.text());
} else {
parser.skipChildren();
Copy link
Member

Choose a reason for hiding this comment

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

is this needed here? I think it does something only when the current token is start array or start object.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, and in other cases it's a noop. But here you could have arrays of objects (see ExportException)

}
}
} else if (token == XContentParser.Token.START_OBJECT) {
parser.skipChildren();
}
headers.put(currentFieldName, values);
}
}
} else {
// Any additional metadata object added by the metadataToXContent method is ignored
// and skipped, so that the parser does not fail on unknown fields. The parser only
// support metadata key-pairs and metadata arrays of values.
parser.skipChildren();
}
} else if (token == XContentParser.Token.START_ARRAY) {
// Parse the array and add each item to the corresponding list of metadata.
// Arrays of objects are not supported yet and just ignored and skipped.
List<String> values = new ArrayList<>();
while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) {
if (token == XContentParser.Token.VALUE_STRING) {
values.add(parser.text());
} else {
parser.skipChildren();
}
}
if (values.size() > 0) {
if (metadata.containsKey(currentFieldName)) {
values.addAll(metadata.get(currentFieldName));
}
metadata.put(currentFieldName, values);
}
}
}

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

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

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

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, List<String>> header : headers.entrySet()) {
e.addHeader(header.getKey(), 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 +568,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
Loading