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.

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

@@ -49,6 +49,7 @@ import java.util.ArrayList;
 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 class RestClient implements Closeable {
     public void performRequestAsync(String method, String endpoint, Map<String, String> params,
                                     HttpEntity entity, HttpAsyncResponseConsumerFactory httpAsyncResponseConsumerFactory,
                                     ResponseListener responseListener, Header... headers) {
-        URI uri = buildUri(pathPrefix, endpoint, params);
+        Objects.requireNonNull(params, "params must not be null");
+        Map<String, String> 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<Integer> 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<HttpHost> hosts, final HttpRequestBase request,
+                                     final Set<Integer> ignoreErrorCodes,
                                      final HttpAsyncResponseConsumerFactory httpAsyncResponseConsumerFactory,
                                      final FailureTrackingResponseListener listener) {
         final HttpHost host = hosts.next();
@@ -304,7 +334,7 @@ public class RestClient implements Closeable {
                     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 class RestClient implements Closeable {
                         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 class RestClient implements Closeable {
                 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<HttpHost> 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 @@ public class RestClient implements Closeable {
                     } 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 class RestClient implements Closeable {
         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 @@ public class RestClient implements Closeable {
     }
 
     private static URI buildUri(String pathPrefix, String path, Map<String, String> params) {
-        Objects.requireNonNull(params, "params must not be null");
         Objects.requireNonNull(path, "path must not be null");
         try {
             String fullPath;

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

@@ -216,23 +216,45 @@ public class RestClientSingleHostTests extends RestClientTestCase {
      */
     public void testErrorStatusCodes() throws IOException {
         for (String method : getHttpMethods()) {
+            Set<Integer> 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<String, String> 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);
@@ -351,11 +373,10 @@ public class RestClientSingleHostTests extends RestClientTestCase {
     private HttpUriRequest performRandomRequest(String method) throws Exception {
         String uriAsString = "/" + randomStatusCode(getRandom());
         URIBuilder uriBuilder = new URIBuilder(uriAsString);
-        Map<String, String> params = Collections.emptyMap();
+        final Map<String, String> 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);
@@ -363,6 +384,14 @@ public class RestClientSingleHostTests extends RestClientTestCase {
                 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;
@@ -433,16 +462,25 @@ public class RestClientSingleHostTests extends RestClientTestCase {
     }
 
     private Response performRequest(String method, String endpoint, Header... headers) throws IOException {
-        switch(randomIntBetween(0, 2)) {
+        return performRequest(method, endpoint, Collections.<String, String>emptyMap(), headers);
+    }
+
+    private Response performRequest(String method, String endpoint, Map<String, String> 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.<String, String>emptyMap(), headers);
+                return restClient.performRequest(method, endpoint, params, headers);
             case 2:
-                return restClient.performRequest(method, endpoint, Collections.<String, String>emptyMap(), (HttpEntity)null, headers);
+                return restClient.performRequest(method, endpoint, params, (HttpEntity)null, headers);
             default:
                 throw new UnsupportedOperationException();
         }
     }
-
 }

client/test/src/main/java/org/elasticsearch/client/RestClientTestUtil.java

@@ -59,7 +59,7 @@ final class RestClientTestUtil {
     }
 
     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) {

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

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestClient.java

@@ -40,8 +40,6 @@ import org.elasticsearch.test.rest.yaml.restspec.ClientYamlSuiteRestSpec;
 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<String> 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 class ClientYamlTestClient {
             }
         }
 
-        List<Integer> ignores = new ArrayList<>();
-        Map<String, String> 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<String, String> pathParts = new HashMap<>();
         Map<String, String> queryStringParams = new HashMap<>();
-        for (Map.Entry<String, String> entry : requestParams.entrySet()) {
+        for (Map.Entry<String, String> entry : params.entrySet()) {
             if (restApi.getPathParts().contains(entry.getKey())) {
                 pathParts.put(entry.getKey(), entry.getValue());
             } else {
@@ -197,9 +176,6 @@ public class ClientYamlTestClient {
             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);
         }
     }