Skip to content

Commit 54b4020

Browse files
authored
HLRC: Fix strict setting exception handling (#38112)
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 Relates #37247
1 parent 4267b8f commit 54b4020

File tree

6 files changed

+142
-7
lines changed

6 files changed

+142
-7
lines changed

client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2020,15 +2020,16 @@ protected final ElasticsearchStatusException parseResponseException(ResponseExce
20202020
Response response = responseException.getResponse();
20212021
HttpEntity entity = response.getEntity();
20222022
ElasticsearchStatusException elasticsearchException;
2023+
RestStatus restStatus = RestStatus.fromCode(response.getStatusLine().getStatusCode());
2024+
20232025
if (entity == null) {
20242026
elasticsearchException = new ElasticsearchStatusException(
2025-
responseException.getMessage(), RestStatus.fromCode(response.getStatusLine().getStatusCode()), responseException);
2027+
responseException.getMessage(), restStatus, responseException);
20262028
} else {
20272029
try {
20282030
elasticsearchException = parseEntity(entity, BytesRestResponse::errorFromXContent);
20292031
elasticsearchException.addSuppressed(responseException);
20302032
} catch (Exception e) {
2031-
RestStatus restStatus = RestStatus.fromCode(response.getStatusLine().getStatusCode());
20322033
elasticsearchException = new ElasticsearchStatusException("Unable to parse response body", restStatus, responseException);
20332034
elasticsearchException.addSuppressed(e);
20342035
}
Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
/*
2+
* Licensed to Elasticsearch under one or more contributor
3+
* license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright
5+
* ownership. Elasticsearch licenses this file to you under
6+
* the Apache License, Version 2.0 (the "License"); you may
7+
* not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
package org.elasticsearch.client;
21+
22+
import org.apache.http.HttpHost;
23+
import org.apache.http.ProtocolVersion;
24+
import org.apache.http.RequestLine;
25+
import org.apache.http.client.methods.HttpGet;
26+
import org.apache.http.message.BasicRequestLine;
27+
import org.apache.http.message.BasicStatusLine;
28+
import org.elasticsearch.test.ESTestCase;
29+
import org.junit.Before;
30+
31+
import java.io.IOException;
32+
import java.util.Collections;
33+
import java.util.List;
34+
35+
import static org.hamcrest.Matchers.equalTo;
36+
import static org.mockito.Matchers.any;
37+
import static org.mockito.Mockito.doThrow;
38+
import static org.mockito.Mockito.mock;
39+
import static org.mockito.Mockito.when;
40+
41+
public class MockRestHighLevelTests extends ESTestCase {
42+
private RestHighLevelClient client;
43+
private static final List<String> WARNINGS = Collections.singletonList("Some Warning");
44+
45+
@Before
46+
private void setupClient() throws IOException {
47+
final RestClient mockClient = mock(RestClient.class);
48+
final Response mockResponse = mock(Response.class);
49+
50+
when(mockResponse.getHost()).thenReturn(new HttpHost("localhost", 9200));
51+
when(mockResponse.getWarnings()).thenReturn(WARNINGS);
52+
53+
ProtocolVersion protocol = new ProtocolVersion("HTTP", 1, 1);
54+
when(mockResponse.getStatusLine()).thenReturn(new BasicStatusLine(protocol, 200, "OK"));
55+
56+
RequestLine requestLine = new BasicRequestLine(HttpGet.METHOD_NAME, "/_blah", protocol);
57+
when(mockResponse.getRequestLine()).thenReturn(requestLine);
58+
59+
WarningFailureException expectedException = new WarningFailureException(mockResponse);
60+
doThrow(expectedException).when(mockClient).performRequest(any());
61+
62+
client = new RestHighLevelClient(mockClient, RestClient::close, Collections.emptyList());
63+
}
64+
65+
public void testWarningFailure() {
66+
WarningFailureException exception = expectThrows(WarningFailureException.class,
67+
() -> client.info(RequestOptions.DEFAULT));
68+
assertThat(exception.getMessage(), equalTo("method [GET], host [http://localhost:9200], URI [/_blah], " +
69+
"status line [HTTP/1.1 200 OK]"));
70+
assertNull(exception.getCause());
71+
assertThat(exception.getResponse().getWarnings(), equalTo(WARNINGS));
72+
}
73+
}

client/rest/src/main/java/org/elasticsearch/client/ResponseException.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@
3232
*/
3333
public final class ResponseException extends IOException {
3434

35-
private Response response;
35+
private final Response response;
3636

3737
public ResponseException(Response response) throws IOException {
3838
super(buildMessage(response));
@@ -49,7 +49,7 @@ public ResponseException(Response response) throws IOException {
4949
this.response = e.getResponse();
5050
}
5151

52-
private static String buildMessage(Response response) throws IOException {
52+
static String buildMessage(Response response) throws IOException {
5353
String message = String.format(Locale.ROOT,
5454
"method [%s], host [%s], URI [%s], status line [%s]",
5555
response.getRequestLine().getMethod(),

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -540,7 +540,7 @@ public void completed(HttpResponse httpResponse) {
540540
if (isSuccessfulResponse(statusCode) || ignoreErrorCodes.contains(response.getStatusLine().getStatusCode())) {
541541
onResponse(node);
542542
if (thisWarningsHandler.warningsShouldFailRequest(response.getWarnings())) {
543-
listener.onDefinitiveFailure(new ResponseException(response));
543+
listener.onDefinitiveFailure(new WarningFailureException(response));
544544
} else {
545545
listener.onSuccess(response);
546546
}
@@ -925,6 +925,9 @@ Response get() throws IOException {
925925
* like the asynchronous API. We wrap the exception so that the caller's
926926
* signature shows up in any exception we throw.
927927
*/
928+
if (exception instanceof WarningFailureException) {
929+
throw new WarningFailureException((WarningFailureException) exception);
930+
}
928931
if (exception instanceof ResponseException) {
929932
throw new ResponseException((ResponseException) exception);
930933
}
Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
/*
2+
* Licensed to Elasticsearch under one or more contributor
3+
* license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright
5+
* ownership. Elasticsearch licenses this file to you under
6+
* the Apache License, Version 2.0 (the "License"); you may
7+
* not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
package org.elasticsearch.client;
21+
22+
import java.io.IOException;
23+
24+
import static org.elasticsearch.client.ResponseException.buildMessage;
25+
26+
/**
27+
* This exception is used to indicate that one or more {@link Response#getWarnings()} exist
28+
* and is typically used when the {@link RestClient} is set to fail by setting
29+
* {@link RestClientBuilder#setStrictDeprecationMode(boolean)} to `true`.
30+
*/
31+
// This class extends RuntimeException in order to deal with wrapping that is done in FutureUtils on exception.
32+
// if the exception is not of type ElasticsearchException or RuntimeException it will be wrapped in a UncategorizedExecutionException
33+
public final class WarningFailureException extends RuntimeException {
34+
35+
private final Response response;
36+
37+
public WarningFailureException(Response response) throws IOException {
38+
super(buildMessage(response));
39+
this.response = response;
40+
}
41+
42+
/**
43+
* Wrap a {@linkplain WarningFailureException} with another one with the current
44+
* stack trace. This is used during synchronous calls so that the caller
45+
* ends up in the stack trace of the exception thrown.
46+
*/
47+
WarningFailureException(WarningFailureException e) {
48+
super(e.getMessage(), e);
49+
this.response = e.getResponse();
50+
}
51+
52+
/**
53+
* Returns the {@link Response} that caused this exception to be thrown.
54+
*/
55+
public Response getResponse() {
56+
return response;
57+
}
58+
}

client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostTests.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -467,9 +467,9 @@ private void assertDeprecationWarnings(List<String> warningHeaderTexts, List<Str
467467
if (expectFailure) {
468468
try {
469469
restClient.performRequest(request);
470-
fail("expected ResponseException from warnings");
470+
fail("expected WarningFailureException from warnings");
471471
return;
472-
} catch (ResponseException e) {
472+
} catch (WarningFailureException e) {
473473
if (false == warningBodyTexts.isEmpty()) {
474474
assertThat(e.getMessage(), containsString("\nWarnings: " + warningBodyTexts));
475475
}

0 commit comments

Comments
 (0)