From 2abe34a2c02391698ffa50128d2befbf8462b9d5 Mon Sep 17 00:00:00 2001 From: Michael Basnight Date: Tue, 8 Jan 2019 19:58:34 -0600 Subject: [PATCH 1/4] HLRC: Fix strict setting exception handling The LLRC's exception handling for strict mode was previously throwing an exception the HLRC assumed was an error response. This is not the case if the result is valid in strict mode, as it will return the proper response wrapped in an exception with warnings. This commit fixes the HLRC such that it no longer spews if it encounters a strict LLRC response. Closes #37090 --- .../client/RestHighLevelClient.java | 10 ++- .../client/MockRestHighLevelTests.java | 75 +++++++++++++++++++ .../client/ResponseException.java | 2 +- .../org/elasticsearch/client/RestClient.java | 5 +- .../client/WarningFailureException.java | 32 ++++++++ .../client/RestClientSingleHostTests.java | 4 +- 6 files changed, 121 insertions(+), 7 deletions(-) create mode 100644 client/rest-high-level/src/test/java/org/elasticsearch/client/MockRestHighLevelTests.java create mode 100644 client/rest/src/main/java/org/elasticsearch/client/WarningFailureException.java diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java index a9c6901d9820a..69d0dd89994c3 100644 --- a/client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java +++ b/client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java @@ -1671,15 +1671,19 @@ protected final ElasticsearchStatusException parseResponseException(ResponseExce Response response = responseException.getResponse(); HttpEntity entity = response.getEntity(); ElasticsearchStatusException elasticsearchException; - if (entity == null) { + RestStatus restStatus = RestStatus.fromCode(response.getStatusLine().getStatusCode()); + + if (responseException instanceof WarningFailureException) { + elasticsearchException = new ElasticsearchStatusException("Warnings/Deprecations caused response to fail", restStatus, + responseException); + } else if (entity == null) { elasticsearchException = new ElasticsearchStatusException( - responseException.getMessage(), RestStatus.fromCode(response.getStatusLine().getStatusCode()), responseException); + responseException.getMessage(), restStatus, responseException); } else { try { elasticsearchException = parseEntity(entity, BytesRestResponse::errorFromXContent); elasticsearchException.addSuppressed(responseException); } catch (Exception e) { - RestStatus restStatus = RestStatus.fromCode(response.getStatusLine().getStatusCode()); elasticsearchException = new ElasticsearchStatusException("Unable to parse response body", restStatus, responseException); elasticsearchException.addSuppressed(e); } diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/MockRestHighLevelTests.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/MockRestHighLevelTests.java new file mode 100644 index 0000000000000..9ecdd648336d2 --- /dev/null +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/MockRestHighLevelTests.java @@ -0,0 +1,75 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.client; + +import org.apache.http.HttpHost; +import org.apache.http.ProtocolVersion; +import org.apache.http.RequestLine; +import org.apache.http.client.methods.HttpGet; +import org.apache.http.message.BasicRequestLine; +import org.apache.http.message.BasicStatusLine; +import org.elasticsearch.ElasticsearchStatusException; +import org.elasticsearch.test.ESTestCase; +import org.junit.Before; + +import java.io.IOException; +import java.util.Collections; +import java.util.List; + +import static org.hamcrest.Matchers.equalTo; +import static org.mockito.Matchers.any; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +public class MockRestHighLevelTests extends ESTestCase { + private RestHighLevelClient client; + private WarningFailureException expectedException; + private static final List WARNINGS = Collections.singletonList("Some Warning"); + + @Before + private void setupClient() throws IOException { + final RestClient mockClient = mock(RestClient.class); + final Response mockResponse = mock(Response.class); + + when(mockResponse.getHost()).thenReturn(new HttpHost("localhost", 9200)); + when(mockResponse.getWarnings()).thenReturn(WARNINGS); + + ProtocolVersion protocol = new ProtocolVersion("HTTP", 1, 1); + when(mockResponse.getStatusLine()).thenReturn(new BasicStatusLine(protocol, 200, "OK")); + + RequestLine requestLine = new BasicRequestLine(HttpGet.METHOD_NAME, "/_blah", protocol); + when(mockResponse.getRequestLine()).thenReturn(requestLine); + + expectedException = new WarningFailureException(mockResponse); + doThrow(expectedException).when(mockClient).performRequest(any()); + + client = new RestHighLevelClient(mockClient, RestClient::close, Collections.emptyList()); + } + + public void testWarningFailure() { + ElasticsearchStatusException exception = expectThrows(ElasticsearchStatusException.class, + () -> client.info(RequestOptions.DEFAULT)); + assertThat(exception.getMessage(), equalTo("Warnings/Deprecations caused response to fail")); + assertThat(exception.getCause(), equalTo(expectedException)); + WarningFailureException warningFailureException = (WarningFailureException) exception.getCause(); + assertThat(warningFailureException.getResponse().getWarnings(), equalTo(WARNINGS)); + } +} diff --git a/client/rest/src/main/java/org/elasticsearch/client/ResponseException.java b/client/rest/src/main/java/org/elasticsearch/client/ResponseException.java index 0957e25fb7033..71b545e6ed9eb 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/ResponseException.java +++ b/client/rest/src/main/java/org/elasticsearch/client/ResponseException.java @@ -30,7 +30,7 @@ * Exception thrown when an elasticsearch node responds to a request with a status code that indicates an error. * Holds the response that was returned. */ -public final class ResponseException extends IOException { +public class ResponseException extends IOException { private Response response; diff --git a/client/rest/src/main/java/org/elasticsearch/client/RestClient.java b/client/rest/src/main/java/org/elasticsearch/client/RestClient.java index 3b1946ef9ed58..6f3541b1ac288 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/RestClient.java +++ b/client/rest/src/main/java/org/elasticsearch/client/RestClient.java @@ -301,7 +301,7 @@ public void completed(HttpResponse httpResponse) { if (isSuccessfulResponse(statusCode) || ignoreErrorCodes.contains(response.getStatusLine().getStatusCode())) { onResponse(node); if (thisWarningsHandler.warningsShouldFailRequest(response.getWarnings())) { - listener.onDefinitiveFailure(new ResponseException(response)); + listener.onDefinitiveFailure(new WarningFailureException(response)); } else { listener.onSuccess(response); } @@ -686,6 +686,9 @@ Response get() throws IOException { * like the asynchronous API. We wrap the exception so that the caller's * signature shows up in any exception we throw. */ + if (exception instanceof WarningFailureException) { + throw new WarningFailureException((WarningFailureException) exception); + } if (exception instanceof ResponseException) { throw new ResponseException((ResponseException) exception); } diff --git a/client/rest/src/main/java/org/elasticsearch/client/WarningFailureException.java b/client/rest/src/main/java/org/elasticsearch/client/WarningFailureException.java new file mode 100644 index 0000000000000..b9648a7ac83c4 --- /dev/null +++ b/client/rest/src/main/java/org/elasticsearch/client/WarningFailureException.java @@ -0,0 +1,32 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.client; + +import java.io.IOException; + +public class WarningFailureException extends ResponseException { + public WarningFailureException(Response response) throws IOException { + super(response); + } + + public WarningFailureException(ResponseException e) throws IOException { + super(e); + } +} diff --git a/client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostTests.java b/client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostTests.java index a37cfe87ca1c8..aaef5404f2802 100644 --- a/client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostTests.java +++ b/client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostTests.java @@ -421,9 +421,9 @@ private void assertDeprecationWarnings(List warningHeaderTexts, List Date: Wed, 23 Jan 2019 12:09:01 -0600 Subject: [PATCH 2/4] Just throw the exception itself --- .../client/RestHighLevelClient.java | 5 +-- .../client/ResponseException.java | 2 +- .../client/WarningFailureException.java | 34 ++++++++++++++++--- 3 files changed, 32 insertions(+), 9 deletions(-) diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java index 69d0dd89994c3..dea0216d320c6 100644 --- a/client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java +++ b/client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java @@ -1673,10 +1673,7 @@ protected final ElasticsearchStatusException parseResponseException(ResponseExce ElasticsearchStatusException elasticsearchException; RestStatus restStatus = RestStatus.fromCode(response.getStatusLine().getStatusCode()); - if (responseException instanceof WarningFailureException) { - elasticsearchException = new ElasticsearchStatusException("Warnings/Deprecations caused response to fail", restStatus, - responseException); - } else if (entity == null) { + if (entity == null) { elasticsearchException = new ElasticsearchStatusException( responseException.getMessage(), restStatus, responseException); } else { diff --git a/client/rest/src/main/java/org/elasticsearch/client/ResponseException.java b/client/rest/src/main/java/org/elasticsearch/client/ResponseException.java index 71b545e6ed9eb..c928438b6aba4 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/ResponseException.java +++ b/client/rest/src/main/java/org/elasticsearch/client/ResponseException.java @@ -49,7 +49,7 @@ public ResponseException(Response response) throws IOException { this.response = e.getResponse(); } - private static String buildMessage(Response response) throws IOException { + static String buildMessage(Response response) throws IOException { String message = String.format(Locale.ROOT, "method [%s], host [%s], URI [%s], status line [%s]", response.getRequestLine().getMethod(), diff --git a/client/rest/src/main/java/org/elasticsearch/client/WarningFailureException.java b/client/rest/src/main/java/org/elasticsearch/client/WarningFailureException.java index b9648a7ac83c4..cdae8d00bef8a 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/WarningFailureException.java +++ b/client/rest/src/main/java/org/elasticsearch/client/WarningFailureException.java @@ -21,12 +21,38 @@ import java.io.IOException; -public class WarningFailureException extends ResponseException { +import static org.elasticsearch.client.ResponseException.buildMessage; + +/** + * This exception is used to indicate that one or more {@link Response#getWarnings()} exist + * and is typically used when the {@link RestClient} is set to fail by setting + * {@link RestClientBuilder#setStrictDeprecationMode(boolean)} to `true`. + */ +// This class extends RuntimeException in order to deal with wrapping that is done in FutureUtils on exception. +// if the exception is not of type ElasticsearchException or RuntimeException it will be wrapped in a UncategorizedExecutionException +public class WarningFailureException extends RuntimeException { + + private Response response; + public WarningFailureException(Response response) throws IOException { - super(response); + super(buildMessage(response)); + this.response = response; + } + + /** + * Wrap a {@linkplain WarningFailureException} with another one with the current + * stack trace. This is used during synchronous calls so that the caller + * ends up in the stack trace of the exception thrown. + */ + WarningFailureException(WarningFailureException e) { + super(e.getMessage(), e); + this.response = e.getResponse(); } - public WarningFailureException(ResponseException e) throws IOException { - super(e); + /** + * Returns the {@link Response} that caused this exception to be thrown. + */ + public Response getResponse() { + return response; } } From 64f9e668daea0aad4c19016f49ff7395f7334b70 Mon Sep 17 00:00:00 2001 From: Michael Basnight Date: Mon, 28 Jan 2019 10:25:34 -0600 Subject: [PATCH 3/4] fixups test + review --- .../elasticsearch/client/MockRestHighLevelTests.java | 11 +++++------ .../org/elasticsearch/client/ResponseException.java | 4 ++-- .../elasticsearch/client/WarningFailureException.java | 4 ++-- 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/MockRestHighLevelTests.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/MockRestHighLevelTests.java index 9ecdd648336d2..7f3ad23e3e8c1 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/MockRestHighLevelTests.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/MockRestHighLevelTests.java @@ -25,7 +25,6 @@ import org.apache.http.client.methods.HttpGet; import org.apache.http.message.BasicRequestLine; import org.apache.http.message.BasicStatusLine; -import org.elasticsearch.ElasticsearchStatusException; import org.elasticsearch.test.ESTestCase; import org.junit.Before; @@ -65,11 +64,11 @@ private void setupClient() throws IOException { } public void testWarningFailure() { - ElasticsearchStatusException exception = expectThrows(ElasticsearchStatusException.class, + WarningFailureException exception = expectThrows(WarningFailureException.class, () -> client.info(RequestOptions.DEFAULT)); - assertThat(exception.getMessage(), equalTo("Warnings/Deprecations caused response to fail")); - assertThat(exception.getCause(), equalTo(expectedException)); - WarningFailureException warningFailureException = (WarningFailureException) exception.getCause(); - assertThat(warningFailureException.getResponse().getWarnings(), equalTo(WARNINGS)); + assertThat(exception.getMessage(), equalTo("method [GET], host [http://localhost:9200], URI [/_blah], " + + "status line [HTTP/1.1 200 OK]")); + assertNull(exception.getCause()); + assertThat(exception.getResponse().getWarnings(), equalTo(WARNINGS)); } } diff --git a/client/rest/src/main/java/org/elasticsearch/client/ResponseException.java b/client/rest/src/main/java/org/elasticsearch/client/ResponseException.java index c928438b6aba4..4d57f12742e03 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/ResponseException.java +++ b/client/rest/src/main/java/org/elasticsearch/client/ResponseException.java @@ -30,9 +30,9 @@ * Exception thrown when an elasticsearch node responds to a request with a status code that indicates an error. * Holds the response that was returned. */ -public class ResponseException extends IOException { +public final class ResponseException extends IOException { - private Response response; + private final Response response; public ResponseException(Response response) throws IOException { super(buildMessage(response)); diff --git a/client/rest/src/main/java/org/elasticsearch/client/WarningFailureException.java b/client/rest/src/main/java/org/elasticsearch/client/WarningFailureException.java index cdae8d00bef8a..1cdadcc13cabc 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/WarningFailureException.java +++ b/client/rest/src/main/java/org/elasticsearch/client/WarningFailureException.java @@ -30,9 +30,9 @@ */ // This class extends RuntimeException in order to deal with wrapping that is done in FutureUtils on exception. // if the exception is not of type ElasticsearchException or RuntimeException it will be wrapped in a UncategorizedExecutionException -public class WarningFailureException extends RuntimeException { +public final class WarningFailureException extends RuntimeException { - private Response response; + private final Response response; public WarningFailureException(Response response) throws IOException { super(buildMessage(response)); From 9ba7b44d2bbde93721eaa630a6a2ce47d47a796f Mon Sep 17 00:00:00 2001 From: Michael Basnight Date: Mon, 28 Jan 2019 10:38:23 -0600 Subject: [PATCH 4/4] move variable --- .../java/org/elasticsearch/client/MockRestHighLevelTests.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/MockRestHighLevelTests.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/MockRestHighLevelTests.java index 7f3ad23e3e8c1..1c7c98cda829c 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/MockRestHighLevelTests.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/MockRestHighLevelTests.java @@ -40,7 +40,6 @@ public class MockRestHighLevelTests extends ESTestCase { private RestHighLevelClient client; - private WarningFailureException expectedException; private static final List WARNINGS = Collections.singletonList("Some Warning"); @Before @@ -57,7 +56,7 @@ private void setupClient() throws IOException { RequestLine requestLine = new BasicRequestLine(HttpGet.METHOD_NAME, "/_blah", protocol); when(mockResponse.getRequestLine()).thenReturn(requestLine); - expectedException = new WarningFailureException(mockResponse); + WarningFailureException expectedException = new WarningFailureException(mockResponse); doThrow(expectedException).when(mockClient).performRequest(any()); client = new RestHighLevelClient(mockClient, RestClient::close, Collections.emptyList());