Add parsing method for ElasticsearchException.generateThrowableXContent() #22783

tlrx · 2017-01-25T10:09:41Z

The output of the ElasticsearchException.generateThrowableXContent() method can be parsed back by the ElasticsearchException.fromXContent() method.

This commit adds unit tests in the style of the to-and-from-xcontent tests we already have for other parsing methods. It also relax the strict parsing of the ElasticsearchException.fromXContent() so that it does not throw an exception when custom metadata of type array & objects are parsed.

javanna

thanks @tlrx I left some comments

javanna · 2017-01-25T12:54:30Z

core/src/main/java/org/elasticsearch/ElasticsearchException.java

+        Map<String, List<String>> metadata = new HashMap<>();
+        Map<String, Object> headers = new HashMap<>();
+
+        XContentParser.Token token = parser.currentToken();


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.

Oh, I didn't address this point yet.

javanna · 2017-01-25T12:54:52Z

core/src/main/java/org/elasticsearch/ElasticsearchException.java

+                token = parser.nextToken();
+            }
+
+            if ( token != null && token.isValue()) {


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

javanna · 2017-01-25T12:56:30Z

core/src/main/java/org/elasticsearch/ElasticsearchException.java

+                } else if (STACK_TRACE.equals(currentFieldName)) {
+                    stack = parser.text();
+                } else {
+                    metadata.put(currentFieldName, Collections.singletonList(parser.text()));


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?

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

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

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.

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.

javanna · 2017-01-25T13:02:01Z

core/src/main/java/org/elasticsearch/ElasticsearchException.java

+                if (CAUSED_BY.equals(currentFieldName)) {
+                    cause = fromXContent(parser);
+                } else if (HEADER.equals(currentFieldName)) {
+                    headers.putAll(parser.map());


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

javanna · 2017-01-25T13:02:42Z

core/src/main/java/org/elasticsearch/ElasticsearchException.java

+                    // and skipped, so that the parser does not fail on unknown fields.
+                    parser.skipChildren();
+                }
+            }else if (token == XContentParser.Token.START_ARRAY) {


can you add a whitespace after the bracket please?

javanna · 2017-01-25T13:12:39Z

core/src/test/java/org/elasticsearch/ElasticsearchExceptionTests.java

+        final Throwable throwable = exceptions.v1();
+
+        BytesReference throwableBytes = XContentHelper.toXContent((b, p) -> {
+            ElasticsearchException.generateThrowableXContent(b, ToXContent.EMPTY_PARAMS, throwable);


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.

javanna · 2017-01-25T13:15:03Z

core/src/test/java/org/elasticsearch/ElasticsearchExceptionTests.java

+                expected.addHeader("doc_id", "test");
+                break;
+            default:
+                throw new IllegalArgumentException("No randomized exceptions generated for type [" + type + "]");


nit: throw UnsupportedOperationException instead?

javanna · 2017-01-25T13:16:57Z

core/src/test/java/org/elasticsearch/ElasticsearchExceptionTests.java

+    public void testThrowableToAndFromXContent() throws IOException {
+        final XContent xContent = randomFrom(XContentType.values()).xContent();
+
+        final Tuple<Throwable, ElasticsearchException> exceptions = randomExceptions();


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.

javanna · 2017-01-25T13:18:02Z

core/src/test/java/org/elasticsearch/ElasticsearchExceptionTests.java

+                expectedCause.addMetadata("es.col", "42");
+
+                expected = new ElasticsearchException("Elasticsearch exception [type=exception, reason=Parsing failed]", expectedCause);
+                expected.addHeader("doc_id", "test");


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

javanna · 2017-01-25T13:19:51Z

core/src/test/java/org/elasticsearch/action/search/SearchPhaseExecutionExceptionTests.java

+        assertNotNull(parsedException);
+        assertThat(parsedException.getHeaderKeys(), hasSize(0));
+        assertThat(parsedException.getMetadataKeys(), hasSize(2));
+        assertThat(parsedException.getMetadata("es.phase"), hasItem(phase));


the es. prefix here is a bit weird, but I don't know how to remove it without breaking bw comp. We have to take into account though that even if the info is available, no users will know how to retrieve it :)

They can still have a look of the content of getMetadataKeys?

right they can iterate over them, that is acceptable.

tlrx · 2017-01-25T15:40:12Z

Thanks @javanna, I missed an important point concerning metadata/header list of values. I updated the code according to your comments, can you have another look please?

javanna

left a few minors, feel free to push once those are addressed.

javanna · 2017-01-25T17:20:55Z

core/src/main/java/org/elasticsearch/ElasticsearchException.java

+                // 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) {


out of curiosity, why supporting null?

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

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.

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?

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 :)

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

javanna · 2017-01-25T17:22:51Z

core/src/main/java/org/elasticsearch/ElasticsearchException.java

+        if (stack != null) {
+            message.append(", ").append(STACK_TRACE).append('=').append(stack);
+        }
+        message.append(']');


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.

javanna · 2017-01-25T17:27:44Z

core/src/test/java/org/elasticsearch/ElasticsearchExceptionTests.java

+        assertNotNull(parsedException);
+
+        ElasticsearchException expected = exceptions.v2();
+        while (expected != null) {


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

javanna · 2017-01-25T17:29:36Z

core/src/test/java/org/elasticsearch/ElasticsearchExceptionTests.java

+                throw new UnsupportedOperationException("No randomized exceptions generated for type [" + type + "]");
+        }
+
+        if (actual instanceof ElasticsearchException) {


you can probably cast it here and reuse it later

javanna · 2017-01-25T17:30:34Z

core/src/test/java/org/elasticsearch/ElasticsearchExceptionTests.java

+            if (randomBoolean()) {
+                Map<String, List<String>> randomHeaders = new HashMap<>();
+
+                int nbHeaders = randomIntBetween(1, 5);


super minor nit: in general, when creating maps and lists and knowing the size you could pass it in.

javanna · 2017-01-25T17:32:28Z

core/src/test/java/org/elasticsearch/ElasticsearchExceptionTests.java

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


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

javanna · 2017-01-25T17:34:26Z

core/src/main/java/org/elasticsearch/ElasticsearchException.java

+                if (CAUSED_BY.equals(currentFieldName)) {
+                    cause = fromXContent(parser);
+                } else if (HEADER.equals(currentFieldName)) {
+                    Map<String, Object> headerMap = parser.map();


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.

javanna · 2017-01-25T17:35:28Z

core/src/main/java/org/elasticsearch/ElasticsearchException.java

+                token = parser.nextToken();
+            }
+
+            if (token != null && token.isValue()) {


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

Right, I removed that.

javanna · 2017-01-25T17:35:55Z

core/src/main/java/org/elasticsearch/ElasticsearchException.java

+        Map<String, List<String>> metadata = new HashMap<>();
+        Map<String, List<String>> headers = new HashMap<>();
+
+        XContentParser.Token token = parser.currentToken();


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.

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.

javanna · 2017-01-26T12:16:52Z

core/src/main/java/org/elasticsearch/ElasticsearchException.java

+                } else if (STACK_TRACE.equals(currentFieldName)) {
+                    stack = parser.text();
+                } else {
+                    metadata.put(currentFieldName, Collections.singletonList(parser.text()));


shall we also skip non strings here?

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.

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.

Let's do that then

javanna · 2017-01-26T12:18:14Z

core/src/test/java/org/elasticsearch/ElasticsearchExceptionTests.java

+                    expected.setResources(resourceType, resourceIds);
+                }
+            }
+        }


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

I added a dedicated test for this.

wonderful, thanks

javanna · 2017-01-26T13:04:29Z

core/src/main/java/org/elasticsearch/ElasticsearchException.java

+                                    if (token == XContentParser.Token.VALUE_STRING) {
+                                        values.add(parser.text());
+                                    } else {
+                                        parser.skipChildren();


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

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

…nt() (#22783) The output of the ElasticsearchException.generateThrowableXContent() method can be parsed back by the ElasticsearchException.fromXContent() method. This commit adds unit tests in the style of the to-and-from-xcontent tests we already have for other parsing methods. It also relax the strict parsing of the ElasticsearchException.fromXContent() so that it does not throw an exception when custom metadata and headers are parsed, as long as they are either strings or arrays of strings. Every other type is ignored at parsing time.

tlrx · 2017-01-26T14:36:06Z

Thanks a lot @javanna - this wasn't fun to review at all.

5.x: e7a6cd4

* master: (47 commits) Remove non needed import use expectThrows instead of manually testing exception Fix checkstyle and a test Update after review Read ec2 discovery address from aws instance tags Invalidate cached query results if query timed out (elastic#22807) Add remaining generated painless API Generate reference links for painless API (elastic#22775) [TEST] Fix ElasticsearchExceptionTests Add parsing method for ElasticsearchException.generateThrowableXContent() (elastic#22783) Improve connection closing in `RemoteClusterConnection` (elastic#22804) Docs: Cluster allocation explain should be on one page Remove DFS_QUERY_AND_FETCH as a search type (elastic#22787) Add repository-url module and move URLRepository (elastic#22752) fix date-processor to a new default year for every new pipeline execution. (elastic#22601) Add tests for top_hits aggregation (elastic#22754) [TEST] Added this for 93a28b0 submitted via elastic#22772 Fix typo in comment in OsProbe.java Add new ruby search library to community clients doc (elastic#22765) RangeQuery WITHIN case now normalises query (elastic#22431) ...

tlrx added 2 commits January 25, 2017 09:25

Move up fromXContent() method

7c079f1

Make fromXContent not fail on unknown array/object metadata

5d32a94

tlrx added :Java High Level REST Client v5.3.0 v6.0.0-alpha1 labels Jan 25, 2017

tlrx requested review from javanna and cbuescher January 25, 2017 10:09

tlrx mentioned this pull request Jan 25, 2017

[WIP] Add headers parsing to ElasticsearchException #22675

Closed

clintongormley added the >enhancement label Jan 25, 2017

javanna requested changes Jan 25, 2017

View reviewed changes

Update after Luca review

5ac4801

javanna approved these changes Jan 25, 2017

View reviewed changes

tlrx added 2 commits January 26, 2017 10:18

Update after Luca review (2)

0ac62a8

Only parse string values, do not parse nulls

4d43094

javanna reviewed Jan 26, 2017

View reviewed changes

Only parse strings in metadata

320023c

javanna reviewed Jan 26, 2017

View reviewed changes

tlrx merged commit be96278 into elastic:master Jan 26, 2017

tlrx removed the review label Jan 26, 2017

tlrx deleted the add-parsing-tests-for-generate-throwable-to-xcontent branch January 26, 2017 14:37

Add parsing method for ElasticsearchException.generateThrowableXContent() #22783

Add parsing method for ElasticsearchException.generateThrowableXContent() #22783

Conversation

tlrx commented Jan 25, 2017

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlrx commented Jan 25, 2017

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlrx commented Jan 26, 2017