From 9da8531f60f092f83f06ba31e90ea64ad8473f9c Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Tue, 28 Feb 2017 09:33:51 +0100 Subject: [PATCH] RestClient asynchronous execution should not throw exceptions (#23307) The current implementation of RestClient.performAsync() methods can throw exceptions before the request is asynchronously executed. Since it only throws unchecked exceptions, it's easy for the user/dev to forget to catch them. Instead I think async methods should never throw exceptions and should always call the listener onFailure() method. --- .../org/elasticsearch/client/RestClient.java | 64 +++++++------- .../elasticsearch/client/RestClientTests.java | 84 +++++++++++++++++++ 2 files changed, 118 insertions(+), 30 deletions(-) create mode 100644 client/rest/src/test/java/org/elasticsearch/client/RestClientTests.java 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 f808c36d60a..ba3a07454ee 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/RestClient.java +++ b/client/rest/src/main/java/org/elasticsearch/client/RestClient.java @@ -289,40 +289,44 @@ public class RestClient implements Closeable { public void performRequestAsync(String method, String endpoint, Map params, HttpEntity entity, HttpAsyncResponseConsumerFactory httpAsyncResponseConsumerFactory, ResponseListener responseListener, Header... headers) { - Objects.requireNonNull(params, "params must not be null"); - Map requestParams = new HashMap<>(params); - //ignore is a special parameter supported by the clients, shouldn't be sent to es - String ignoreString = requestParams.remove("ignore"); - Set ignoreErrorCodes; - if (ignoreString == null) { - if (HttpHead.METHOD_NAME.equals(method)) { - //404 never causes error if returned for a HEAD request - ignoreErrorCodes = Collections.singleton(404); + try { + Objects.requireNonNull(params, "params must not be null"); + Map requestParams = new HashMap<>(params); + //ignore is a special parameter supported by the clients, shouldn't be sent to es + String ignoreString = requestParams.remove("ignore"); + Set ignoreErrorCodes; + if (ignoreString == null) { + if (HttpHead.METHOD_NAME.equals(method)) { + //404 never causes error if returned for a HEAD request + ignoreErrorCodes = Collections.singleton(404); + } else { + ignoreErrorCodes = Collections.emptySet(); + } } else { - ignoreErrorCodes = Collections.emptySet(); - } - } else { - String[] ignoresArray = ignoreString.split(","); - ignoreErrorCodes = new HashSet<>(); - if (HttpHead.METHOD_NAME.equals(method)) { - //404 never causes error if returned for a HEAD request - ignoreErrorCodes.add(404); - } - for (String ignoreCode : ignoresArray) { - try { - ignoreErrorCodes.add(Integer.valueOf(ignoreCode)); - } catch (NumberFormatException e) { - throw new IllegalArgumentException("ignore value should be a number, found [" + ignoreString + "] instead", e); + String[] ignoresArray = ignoreString.split(","); + ignoreErrorCodes = new HashSet<>(); + if (HttpHead.METHOD_NAME.equals(method)) { + //404 never causes error if returned for a HEAD request + ignoreErrorCodes.add(404); + } + for (String ignoreCode : ignoresArray) { + try { + ignoreErrorCodes.add(Integer.valueOf(ignoreCode)); + } catch (NumberFormatException e) { + throw new IllegalArgumentException("ignore value should be a number, found [" + ignoreString + "] instead", e); + } } } + URI uri = buildUri(pathPrefix, endpoint, requestParams); + HttpRequestBase request = createHttpRequest(method, uri, entity); + setHeaders(request, headers); + FailureTrackingResponseListener failureTrackingResponseListener = new FailureTrackingResponseListener(responseListener); + long startTime = System.nanoTime(); + performRequestAsync(startTime, nextHost(), request, ignoreErrorCodes, httpAsyncResponseConsumerFactory, + failureTrackingResponseListener); + } catch (Exception e) { + responseListener.onFailure(e); } - URI uri = buildUri(pathPrefix, endpoint, requestParams); - HttpRequestBase request = createHttpRequest(method, uri, entity); - setHeaders(request, headers); - FailureTrackingResponseListener failureTrackingResponseListener = new FailureTrackingResponseListener(responseListener); - long startTime = System.nanoTime(); - performRequestAsync(startTime, nextHost(), request, ignoreErrorCodes, httpAsyncResponseConsumerFactory, - failureTrackingResponseListener); } private void performRequestAsync(final long startTime, final HostTuple> hostTuple, final HttpRequestBase request, diff --git a/client/rest/src/test/java/org/elasticsearch/client/RestClientTests.java b/client/rest/src/test/java/org/elasticsearch/client/RestClientTests.java new file mode 100644 index 00000000000..1ed08e19ab6 --- /dev/null +++ b/client/rest/src/test/java/org/elasticsearch/client/RestClientTests.java @@ -0,0 +1,84 @@ +/* + * 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.Header; +import org.apache.http.HttpHost; +import org.apache.http.impl.nio.client.CloseableHttpAsyncClient; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; +import static org.mockito.Mockito.mock; + +public class RestClientTests extends RestClientTestCase { + + public void testPerformAsyncWithUnsupportedMethod() throws Exception { + RestClient.SyncResponseListener listener = new RestClient.SyncResponseListener(10000); + try (RestClient restClient = createRestClient()) { + restClient.performRequestAsync("unsupported", randomAsciiOfLength(5), listener); + listener.get(); + + fail("should have failed because of unsupported method"); + } catch (UnsupportedOperationException exception) { + assertEquals("http method not supported: unsupported", exception.getMessage()); + } + } + + public void testPerformAsyncWithNullParams() throws Exception { + RestClient.SyncResponseListener listener = new RestClient.SyncResponseListener(10000); + try (RestClient restClient = createRestClient()) { + restClient.performRequestAsync(randomAsciiOfLength(5), randomAsciiOfLength(5), null, listener); + listener.get(); + + fail("should have failed because of null parameters"); + } catch (NullPointerException exception) { + assertEquals("params must not be null", exception.getMessage()); + } + } + + public void testPerformAsyncWithNullHeaders() throws Exception { + RestClient.SyncResponseListener listener = new RestClient.SyncResponseListener(10000); + try (RestClient restClient = createRestClient()) { + restClient.performRequestAsync("GET", randomAsciiOfLength(5), listener, null); + listener.get(); + + fail("should have failed because of null headers"); + } catch (NullPointerException exception) { + assertEquals("request headers must not be null", exception.getMessage()); + } + } + + public void testPerformAsyncWithWrongEndpoint() throws Exception { + RestClient.SyncResponseListener listener = new RestClient.SyncResponseListener(10000); + try (RestClient restClient = createRestClient()) { + restClient.performRequestAsync("GET", "::http:///", listener); + listener.get(); + + fail("should have failed because of wrong endpoint"); + } catch (IllegalArgumentException exception) { + assertEquals("Expected scheme name at index 0: ::http:///", exception.getMessage()); + } + } + + private static RestClient createRestClient() { + HttpHost[] hosts = new HttpHost[]{new HttpHost("localhost", 9200)}; + return new RestClient(mock(CloseableHttpAsyncClient.class), randomLongBetween(1_000, 30_000), new Header[]{}, hosts, null, null); + } +}