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 3 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
176 changes: 109 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,119 @@ 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, List<String>> headers = new HashMap<>();

XContentParser.Token token = parser.currentToken();
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 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

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

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

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

Choose a reason for hiding this comment

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

Same question as before: how can the token be null? that seems like a bug?

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, I removed that.

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)) {
Map<String, Object> headerMap = parser.map();
Copy link
Member

Choose a reason for hiding this comment

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

you are still casting here and eventually calling toString on Objects. What I meant is manually parse these headers instead and make the parsing code a little more accurate. That also won't require to go through the map once again once parsed.

if (headerMap != null) {
for (Map.Entry<String, Object> header : headerMap.entrySet()) {
List<String> values = headers.getOrDefault(header.getKey(), new ArrayList<>());
if (header.getValue() instanceof Iterable) {
for (Object value : (Iterable) header.getValue()) {
values.add(String.valueOf(value));
}
} else {
values.add(String.valueOf(header.getValue()));
}
headers.put(header.getKey(), 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.isValue() || token == XContentParser.Token.VALUE_NULL) {
Copy link
Member

Choose a reason for hiding this comment

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

out of curiosity, why supporting null?

Copy link
Member

Choose a reason for hiding this comment

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

question: what happens with fields that are not strings? do they become strings?

Copy link
Member Author

Choose a reason for hiding this comment

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

It supports null because headers/metadata/resources can contain null values and those null values will be present in the printed xcontent. So I think we have to parse them and it doesn't add extra complexity.

question: what happens with fields that are not strings? do they become strings?

parser.textOrNull() takes care to retrieve a textual representation of the value but we might lose some precision.

Copy link
Member

Choose a reason for hiding this comment

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

you mean they can contain null values because HashMap supports it or because it actually makes sense to have null values in there? I wonder if we should rather validate values and reject null.

Would it be possible to only parse strings and skip the rest for now?

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 mean they can contain null values because HashMap supports it or because it actually makes sense to have null values in there?

Neither of the two: since we allow nulls to be added and printed I thought it would be better to also be able to parse them back.

I wonder if we should rather validate values and reject null.

Would it be possible to only parse strings and skip the rest for now?

I agree and yes, as long ass we take care of rejecting them in a follow up PR :)

Copy link
Member Author

Choose a reason for hiding this comment

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

4d43094 updates the code so that only strings are parsed.

values.add(parser.textOrNull());
} else {
parser.skipChildren();
}
}
if (values.size() > 0) {
if (metadata.containsKey(currentFieldName)) {
values.addAll(metadata.get(currentFieldName));
}
metadata.put(currentFieldName, values);
}
}
} 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

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 +562,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