From 2cbc6888a2dac2eb577808a379485fd8a5118bd6 Mon Sep 17 00:00:00 2001 From: Michael Basnight Date: Wed, 30 Jan 2019 11:31:59 -0600 Subject: [PATCH] HLRC: Fix strict setting exception handling (#37247) 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 | 5 +- .../client/MockRestHighLevelTests.java | 73 +++++++++++++++++++ .../client/ResponseException.java | 4 +- .../org/elasticsearch/client/RestClient.java | 5 +- .../client/WarningFailureException.java | 58 +++++++++++++++ .../client/RestClientSingleHostTests.java | 4 +- 6 files changed, 142 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 5ef0e0110c1..2eebd0cc56c 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,16 @@ public class RestHighLevelClient implements Closeable { Response response = responseException.getResponse(); HttpEntity entity = response.getEntity(); ElasticsearchStatusException elasticsearchException; + RestStatus restStatus = RestStatus.fromCode(response.getStatusLine().getStatusCode()); + 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 00000000000..1c7c98cda82 --- /dev/null +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/MockRestHighLevelTests.java @@ -0,0 +1,73 @@ +/* + * 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.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 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); + + WarningFailureException expectedException = new WarningFailureException(mockResponse); + doThrow(expectedException).when(mockClient).performRequest(any()); + + client = new RestHighLevelClient(mockClient, RestClient::close, Collections.emptyList()); + } + + public void testWarningFailure() { + WarningFailureException exception = expectThrows(WarningFailureException.class, + () -> client.info(RequestOptions.DEFAULT)); + 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 0957e25fb70..4d57f12742e 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/ResponseException.java +++ b/client/rest/src/main/java/org/elasticsearch/client/ResponseException.java @@ -32,7 +32,7 @@ import java.util.Locale; */ public final class ResponseException extends IOException { - private Response response; + private final Response response; public ResponseException(Response response) throws IOException { super(buildMessage(response)); @@ -49,7 +49,7 @@ public final class ResponseException extends 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/RestClient.java b/client/rest/src/main/java/org/elasticsearch/client/RestClient.java index d053bda7d44..175d524f02a 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 class RestClient implements Closeable { 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 @@ public class RestClient implements Closeable { * 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 00000000000..1cdadcc13ca --- /dev/null +++ b/client/rest/src/main/java/org/elasticsearch/client/WarningFailureException.java @@ -0,0 +1,58 @@ +/* + * 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; + +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 final class WarningFailureException extends RuntimeException { + + private final Response response; + + public WarningFailureException(Response response) throws IOException { + 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(); + } + + /** + * Returns the {@link Response} that caused this exception to be thrown. + */ + public Response getResponse() { + return response; + } +} 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 a37cfe87ca1..aaef5404f28 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 @@ public class RestClientSingleHostTests extends RestClientTestCase { if (expectFailure) { try { restClient.performRequest(request); - fail("expected ResponseException from warnings"); + fail("expected WarningFailureException from warnings"); return; - } catch (ResponseException e) { + } catch (WarningFailureException e) { if (false == warningBodyTexts.isEmpty()) { assertThat(e.getMessage(), containsString("\nWarnings: " + warningBodyTexts)); }