From e79b03bcde49591cbf7e6984aee7ea9b6eb22e4e Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Mon, 16 Jan 2017 18:54:44 +0100 Subject: [PATCH] move ignore parameter support from yaml test client to low level rest client (#22637) All the language clients support a special ignore parameter that doesn't get passed to elasticsearch with the request, but used to indicate which error code should not lead to an exception if returned for a specific request. Moving this to the low level REST client will allow the high level REST client to make use of it too, for instance so that it doesn't have to intercept ResponseExceptions when the get api returns a 404. --- .../org/elasticsearch/client/RestClient.java | 49 +++++++++++--- .../client/RestClientSingleHostTests.java | 64 +++++++++++++++---- .../client/RestClientTestUtil.java | 2 +- docs/java-rest/usage.asciidoc | 9 ++- .../test/rest/yaml/ClientYamlTestClient.java | 28 +------- 5 files changed, 101 insertions(+), 51 deletions(-) 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 89c3309dbbdd1..ce33224b7ecda 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/RestClient.java +++ b/client/rest/src/main/java/org/elasticsearch/client/RestClient.java @@ -49,6 +49,7 @@ import java.util.Collection; import java.util.Collections; import java.util.Comparator; +import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; import java.util.List; @@ -282,15 +283,44 @@ public void performRequestAsync(String method, String endpoint, Map params, HttpEntity entity, HttpAsyncResponseConsumerFactory httpAsyncResponseConsumerFactory, ResponseListener responseListener, Header... headers) { - URI uri = buildUri(pathPrefix, endpoint, params); + 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 { + 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().iterator(), request, httpAsyncResponseConsumerFactory, failureTrackingResponseListener); + performRequestAsync(startTime, nextHost().iterator(), request, ignoreErrorCodes, httpAsyncResponseConsumerFactory, + failureTrackingResponseListener); } private void performRequestAsync(final long startTime, final Iterator hosts, final HttpRequestBase request, + final Set ignoreErrorCodes, final HttpAsyncResponseConsumerFactory httpAsyncResponseConsumerFactory, final FailureTrackingResponseListener listener) { final HttpHost host = hosts.next(); @@ -304,7 +334,7 @@ public void completed(HttpResponse httpResponse) { RequestLogger.logResponse(logger, request, host, httpResponse); int statusCode = httpResponse.getStatusLine().getStatusCode(); Response response = new Response(request.getRequestLine(), host, httpResponse); - if (isSuccessfulResponse(request.getMethod(), statusCode)) { + if (isSuccessfulResponse(statusCode) || ignoreErrorCodes.contains(response.getStatusLine().getStatusCode())) { onResponse(host); listener.onSuccess(response); } else { @@ -312,7 +342,7 @@ public void completed(HttpResponse httpResponse) { if (isRetryStatus(statusCode)) { //mark host dead and retry against next one onFailure(host); - retryIfPossible(responseException, hosts, request); + retryIfPossible(responseException); } else { //mark host alive and don't retry, as the error should be a request problem onResponse(host); @@ -329,13 +359,13 @@ public void failed(Exception failure) { try { RequestLogger.logFailedRequest(logger, request, host, failure); onFailure(host); - retryIfPossible(failure, hosts, request); + retryIfPossible(failure); } catch(Exception e) { listener.onDefinitiveFailure(e); } } - private void retryIfPossible(Exception exception, Iterator hosts, HttpRequestBase request) { + private void retryIfPossible(Exception exception) { if (hosts.hasNext()) { //in case we are retrying, check whether maxRetryTimeout has been reached long timeElapsedMillis = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startTime); @@ -347,7 +377,7 @@ private void retryIfPossible(Exception exception, Iterator hosts, Http } else { listener.trackFailure(exception); request.reset(); - performRequestAsync(startTime, hosts, request, httpAsyncResponseConsumerFactory, listener); + performRequestAsync(startTime, hosts, request, ignoreErrorCodes, httpAsyncResponseConsumerFactory, listener); } } else { listener.onDefinitiveFailure(exception); @@ -452,8 +482,8 @@ public void close() throws IOException { client.close(); } - private static boolean isSuccessfulResponse(String method, int statusCode) { - return statusCode < 300 || (HttpHead.METHOD_NAME.equals(method) && statusCode == 404); + private static boolean isSuccessfulResponse(int statusCode) { + return statusCode < 300; } private static boolean isRetryStatus(int statusCode) { @@ -510,7 +540,6 @@ private static HttpRequestBase addRequestBody(HttpRequestBase httpRequest, HttpE } private static URI buildUri(String pathPrefix, String path, Map params) { - Objects.requireNonNull(params, "params must not be null"); Objects.requireNonNull(path, "path must not be null"); try { String fullPath; 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 ce0d6d0936eeb..a3ba844c7647f 100644 --- a/client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostTests.java +++ b/client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostTests.java @@ -220,23 +220,45 @@ public void testOkStatusCodes() throws IOException { */ public void testErrorStatusCodes() throws IOException { for (String method : getHttpMethods()) { + Set expectedIgnores = new HashSet<>(); + String ignoreParam = ""; + if (HttpHead.METHOD_NAME.equals(method)) { + expectedIgnores.add(404); + } + if (randomBoolean()) { + int numIgnores = randomIntBetween(1, 3); + for (int i = 0; i < numIgnores; i++) { + Integer code = randomFrom(getAllErrorStatusCodes()); + expectedIgnores.add(code); + ignoreParam += code; + if (i < numIgnores - 1) { + ignoreParam += ","; + } + } + } //error status codes should cause an exception to be thrown for (int errorStatusCode : getAllErrorStatusCodes()) { try { - Response response = performRequest(method, "/" + errorStatusCode); - if (method.equals("HEAD") && errorStatusCode == 404) { - //no exception gets thrown although we got a 404 - assertThat(response.getStatusLine().getStatusCode(), equalTo(errorStatusCode)); + Map params; + if (ignoreParam.isEmpty()) { + params = Collections.emptyMap(); + } else { + params = Collections.singletonMap("ignore", ignoreParam); + } + Response response = performRequest(method, "/" + errorStatusCode, params); + if (expectedIgnores.contains(errorStatusCode)) { + //no exception gets thrown although we got an error status code, as it was configured to be ignored + assertEquals(errorStatusCode, response.getStatusLine().getStatusCode()); } else { fail("request should have failed"); } } catch(ResponseException e) { - if (method.equals("HEAD") && errorStatusCode == 404) { + if (expectedIgnores.contains(errorStatusCode)) { throw e; } - assertThat(e.getResponse().getStatusLine().getStatusCode(), equalTo(errorStatusCode)); + assertEquals(errorStatusCode, e.getResponse().getStatusLine().getStatusCode()); } - if (errorStatusCode <= 500) { + if (errorStatusCode <= 500 || expectedIgnores.contains(errorStatusCode)) { failureListener.assertNotCalled(); } else { failureListener.assertCalled(httpHost); @@ -372,11 +394,10 @@ public void testHeaders() throws IOException { private HttpUriRequest performRandomRequest(String method) throws Exception { String uriAsString = "/" + randomStatusCode(getRandom()); URIBuilder uriBuilder = new URIBuilder(uriAsString); - Map params = Collections.emptyMap(); + final Map params = new HashMap<>(); boolean hasParams = randomBoolean(); if (hasParams) { int numParams = randomIntBetween(1, 3); - params = new HashMap<>(numParams); for (int i = 0; i < numParams; i++) { String paramKey = "param-" + i; String paramValue = randomAsciiOfLengthBetween(3, 10); @@ -384,6 +405,14 @@ private HttpUriRequest performRandomRequest(String method) throws Exception { uriBuilder.addParameter(paramKey, paramValue); } } + if (randomBoolean()) { + //randomly add some ignore parameter, which doesn't get sent as part of the request + String ignore = Integer.toString(randomFrom(RestClientTestUtil.getAllErrorStatusCodes())); + if (randomBoolean()) { + ignore += "," + Integer.toString(randomFrom(RestClientTestUtil.getAllErrorStatusCodes())); + } + params.put("ignore", ignore); + } URI uri = uriBuilder.build(); HttpUriRequest request; @@ -455,16 +484,25 @@ private HttpUriRequest performRandomRequest(String method) throws Exception { } private Response performRequest(String method, String endpoint, Header... headers) throws IOException { - switch(randomIntBetween(0, 2)) { + return performRequest(method, endpoint, Collections.emptyMap(), headers); + } + + private Response performRequest(String method, String endpoint, Map params, Header... headers) throws IOException { + int methodSelector; + if (params.isEmpty()) { + methodSelector = randomIntBetween(0, 2); + } else { + methodSelector = randomIntBetween(1, 2); + } + switch(methodSelector) { case 0: return restClient.performRequest(method, endpoint, headers); case 1: - return restClient.performRequest(method, endpoint, Collections.emptyMap(), headers); + return restClient.performRequest(method, endpoint, params, headers); case 2: - return restClient.performRequest(method, endpoint, Collections.emptyMap(), (HttpEntity)null, headers); + return restClient.performRequest(method, endpoint, params, (HttpEntity)null, headers); default: throw new UnsupportedOperationException(); } } - } diff --git a/client/test/src/main/java/org/elasticsearch/client/RestClientTestUtil.java b/client/test/src/main/java/org/elasticsearch/client/RestClientTestUtil.java index 4d4aa00f4929f..fd8046c10fae3 100644 --- a/client/test/src/main/java/org/elasticsearch/client/RestClientTestUtil.java +++ b/client/test/src/main/java/org/elasticsearch/client/RestClientTestUtil.java @@ -55,7 +55,7 @@ static String randomHttpMethod(Random random) { } static int randomStatusCode(Random random) { - return RandomPicks.randomFrom(random, ALL_ERROR_STATUS_CODES); + return RandomPicks.randomFrom(random, ALL_STATUS_CODES); } static int randomOkStatusCode(Random random) { diff --git a/docs/java-rest/usage.asciidoc b/docs/java-rest/usage.asciidoc index b3097ef9d0bd6..69eb16280ed0f 100644 --- a/docs/java-rest/usage.asciidoc +++ b/docs/java-rest/usage.asciidoc @@ -206,7 +206,14 @@ access to the returned response. NOTE: A `ResponseException` is **not** thrown for `HEAD` requests that return a `404` status code because it is an expected `HEAD` response that simply denotes that the resource is not found. All other HTTP methods (e.g., `GET`) -throw a `ResponseException` for `404` responses. +throw a `ResponseException` for `404` responses unless the `ignore` parameter +contains `404`. `ignore` is a special client parameter that doesn't get sent +to Elasticsearch and contains a comma separated list of error status codes. +It allows to control whether some error status code should be treated as an +expected response rather than as an exception. This is useful for instance +with the get api as it can return `404` when the document is missing, in which +case the response body will not contain an error but rather the usual get api +response, just without the document as it was not found. === Example requests diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestClient.java b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestClient.java index 3b719b6d04f10..748a08384ca92 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestClient.java +++ b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestClient.java @@ -40,8 +40,6 @@ import java.io.IOException; import java.net.URI; import java.net.URISyntaxException; -import java.util.ArrayList; -import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -59,7 +57,7 @@ public class ClientYamlTestClient { * Query params that don't need to be declared in the spec, they are supported by default. */ private static final Set ALWAYS_ACCEPTED_QUERY_STRING_PARAMS = Sets.newHashSet( - "error_trace", "filter_path", "human", "pretty", "source"); + "ignore", "error_trace", "human", "filter_path", "pretty", "source"); private final ClientYamlSuiteRestSpec restSpec; private final RestClient restClient; @@ -101,31 +99,12 @@ public ClientYamlTestResponse callApi(String apiName, Map params } } - List ignores = new ArrayList<>(); - Map requestParams; - if (params == null) { - requestParams = Collections.emptyMap(); - } else { - requestParams = new HashMap<>(params); - if (params.isEmpty() == false) { - //ignore is a special parameter supported by the clients, shouldn't be sent to es - String ignoreString = requestParams.remove("ignore"); - if (ignoreString != null) { - try { - ignores.add(Integer.valueOf(ignoreString)); - } catch (NumberFormatException e) { - throw new IllegalArgumentException("ignore value should be a number, found [" + ignoreString + "] instead"); - } - } - } - } - ClientYamlSuiteRestApi restApi = restApi(apiName); //divide params between ones that go within query string and ones that go within path Map pathParts = new HashMap<>(); Map queryStringParams = new HashMap<>(); - for (Map.Entry entry : requestParams.entrySet()) { + for (Map.Entry entry : params.entrySet()) { if (restApi.getPathParts().contains(entry.getKey())) { pathParts.put(entry.getKey(), entry.getValue()); } else { @@ -197,9 +176,6 @@ public ClientYamlTestResponse callApi(String apiName, Map params Response response = restClient.performRequest(requestMethod, requestPath, queryStringParams, requestBody, requestHeaders); return new ClientYamlTestResponse(response); } catch(ResponseException e) { - if (ignores.contains(e.getResponse().getStatusLine().getStatusCode())) { - return new ClientYamlTestResponse(e.getResponse()); - } throw new ClientYamlTestResponseException(e); } }