Stop returning "es." internal exception headers as http response headers #22703

javanna · 2017-01-19T21:23:31Z

Move "es." internal headers to separate metadata set in ElasticsearchException and stop returning them as response headers. These headers are printed out as part of the response body already, they weren't meant to be sent back as response headers too. They were introduced to prevent having to add custom exceptions to our codebase whenever we need to throw an exception that has to hold some additional metadata that ElasticsearchException doesn't support out of the box. The header notion stays in ElasticsearchException but only for what actually needs to be returned as response header (no "es." prefix).

Also removed ESExceptionTests and moved its tests under ElasticsearchExceptionTests or ExceptionSerializationTests

Closes #17593

tlrx

Thanks @javanna, I think this is heading in the right direction. I left some minor comments but we still need to figure out what to do with the TODOs :)

I do like the move of internal headers es.* to exception metadata and I share your concerns about serializing back a parsed exception... but that shouldn't happen, neither in core nor in the high level rest client.

For a long term, I think we should also drop the metadataToXcontent() method which can lead to name conflicts in the rendered xcontent and also adds confusion if we add a metadata class attribute.

tlrx · 2017-01-20T10:31:53Z

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

    }

    /**
     * Adds a new header with the given key.
     * This method will replace existing header if a header with the same key already exists
     */
    public void addHeader(String key, List<String> value) {
+        //we need to enforce this otherwise bw comp doesn't work properly, as "es." was the previous criteria to split headers in two sets
+        if (key.startsWith("es.")) {
+            throw new IllegalArgumentException("exception headers must not start with [es.], found [" + key + "] instead");


Maybe remove "instead" from the exception message?

tlrx · 2017-01-20T10:43:24Z

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

@@ -410,6 +470,8 @@ public static ElasticsearchException fromXContent(XContentParser parser) throws

        String type = null, reason = null, stack = null;
        ElasticsearchException cause = null;
+        Map<String, List<String>> metadata = new HashMap<>();
+        //TODO should this be a Map<String, List<String>> instead?
        Map<String, Object> headers = new HashMap<>();


Yes, I changed it in #22675

ok I will remove the TODO then

tlrx · 2017-01-20T10:44:39Z

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

-                    // Everything else is considered as a header
-                    headers.put(currentFieldName, parser.text());
+                    //TODO here we need to parse multiple values
+                    metadata.put(currentFieldName, Collections.singletonList(parser.text()));


I think it's OK here because the token is a value, not an array. But later in the loop we need to take care of arrays.

I think we can already do it for values and just ignore objects for now

Are we testing this somewhere?

tlrx · 2017-01-20T10:57:47Z

core/src/test/java/org/elasticsearch/rest/BytesRestResponseTests.java

    }

    public void testWithHeaders() throws Exception {
        RestRequest request = new FakeRestRequest();
        RestChannel channel = randomBoolean() ? new DetailedExceptionRestChannel(request) : new SimpleExceptionRestChannel(request);

        BytesRestResponse response = new BytesRestResponse(channel, new WithHeadersException());
+        assertEquals(2, response.getHeaders().size());
+        assertThat(response.getHeaders().get("n1"), notNullValue());


this line is duplicated

javanna · 2017-01-20T14:22:29Z

For a long term, I think we should also drop the metadataToXcontent() method which can lead to name conflicts in the rendered xcontent and also adds confusion if we add a metadata class attribute.

that's correct, I think that the problem is that those additional fields are printed at the same level as the metadata (previous es. headers). They should rather be in a different object that only subclasses can write, but that is a breaking change. That said I am not sure how many clients are able to parse our exceptions entirely given how difficult we made it for them :)

javanna · 2017-01-23T11:50:16Z

@s1monw mind having a look too?

s1monw

LGTM

…Exception and stop returning them as response headers Closes elastic#17593

…ptionTests or ExceptionSerializationTests

…ers (#22703) move "es." internal headers to separate metadata set in ElasticsearchException and stop returning them as response headers Closes #17593 * [TEST] remove ESExceptionTests, move its methods to ElasticsearchExceptionTests or ExceptionSerializationTests

We added version dependent serialziation in #22703 which triggers a bug in LocalTransport that misses to set the serializatoin version when an exception is serialized. This is only relevant for tests since local transport has by-definition the same version on both ends in production. Closes #22784

javanna added :Exceptions >enhancement v5.3.0 v6.0.0-alpha1 labels Jan 19, 2017

javanna requested a review from tlrx January 19, 2017 21:23

javanna force-pushed the enhancement/stop_returning_es_exception_headers branch from 3817f0d to 2bc7547 Compare January 19, 2017 21:26

javanna requested a review from jaymode January 19, 2017 21:26

tlrx reviewed Jan 20, 2017

View reviewed changes

s1monw approved these changes Jan 24, 2017

View reviewed changes

javanna added 3 commits January 24, 2017 12:40

move "es." internal headers to separate metadata set in Elasticsearch…

fe2d23f

…Exception and stop returning them as response headers Closes elastic#17593

[TEST] remove ESExceptionTests, move its methods to ElasticsearchExce…

79964d8

…ptionTests or ExceptionSerializationTests

address some review comments

79a8454

javanna force-pushed the enhancement/stop_returning_es_exception_headers branch from 17ab7eb to 95b9979 Compare January 24, 2017 11:41

jaymode approved these changes Jan 24, 2017

View reviewed changes

update TODO on parsing exceptions back with a potential plan

ea0ee3b

javanna added >bug and removed >enhancement labels Jan 24, 2017

javanna force-pushed the enhancement/stop_returning_es_exception_headers branch from 95b9979 to ea0ee3b Compare January 24, 2017 12:51

javanna merged commit 47c0e13 into elastic:master Jan 24, 2017

tlrx mentioned this pull request Jan 25, 2017

[WIP] Add headers parsing to ElasticsearchException #22675

Closed

talevy mentioned this pull request Dec 8, 2017

Expose failed processor using exception metadata #27724

Closed

lcawl added the :Core/Infra/Core Core issues without another label label Feb 13, 2018

lcawl removed the :Exceptions label Feb 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop returning "es." internal exception headers as http response headers #22703

Stop returning "es." internal exception headers as http response headers #22703

javanna commented Jan 19, 2017 •

edited

Loading

tlrx left a comment

tlrx Jan 20, 2017

tlrx Jan 20, 2017

javanna Jan 23, 2017

tlrx Jan 20, 2017

javanna Jan 23, 2017

tlrx Jan 20, 2017

javanna Jan 23, 2017

javanna commented Jan 20, 2017

javanna commented Jan 23, 2017

s1monw left a comment

Stop returning "es." internal exception headers as http response headers #22703

Stop returning "es." internal exception headers as http response headers #22703

Conversation

javanna commented Jan 19, 2017 • edited Loading

tlrx left a comment

Choose a reason for hiding this comment

tlrx Jan 20, 2017

Choose a reason for hiding this comment

tlrx Jan 20, 2017

Choose a reason for hiding this comment

javanna Jan 23, 2017

Choose a reason for hiding this comment

tlrx Jan 20, 2017

Choose a reason for hiding this comment

javanna Jan 23, 2017

Choose a reason for hiding this comment

tlrx Jan 20, 2017

Choose a reason for hiding this comment

javanna Jan 23, 2017

Choose a reason for hiding this comment

javanna commented Jan 20, 2017

javanna commented Jan 23, 2017

s1monw left a comment

Choose a reason for hiding this comment

javanna commented Jan 19, 2017 •

edited

Loading