Skip to content

Commit

Permalink
move ignore parameter support from yaml test client to low level rest…
Browse files Browse the repository at this point in the history 
… 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.
  • Loading branch information
@javanna
javanna committed Jan 16, 2017
1 parent e28c678 commit e79b03b
Show file tree
Hide file tree
Showing 5 changed files with 101 additions and 51 deletions.
49 changes: 39 additions & 10 deletions client/rest/src/main/java/org/elasticsearch/client/RestClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -282,15 +283,44 @@ public void performRequestAsync(String method, String endpoint, Map<String, Stri
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();
Expand All @@ -304,15 +334,15 @@ 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 {
ResponseException responseException = new ResponseException(response);
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);
Expand All @@ -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<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);
Expand All @@ -347,7 +377,7 @@ private void retryIfPossible(Exception exception, Iterator<HttpHost> 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);
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -510,7 +540,6 @@ private static HttpRequestBase addRequestBody(HttpRequestBase httpRequest, HttpE
}

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;
Expand Down
64 changes: 51 additions & 13 deletions client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -220,23 +220,45 @@ public void testOkStatusCodes() throws IOException {
*/
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);
Expand Down Expand Up @@ -372,18 +394,25 @@ public void testHeaders() throws IOException {
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);
params.put(paramKey, paramValue);
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;
Expand Down Expand Up @@ -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.<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();
}
}

}
2 changes: 1 addition & 1 deletion client/test/src/main/java/org/elasticsearch/client/RestClientTestUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
9 changes: 8 additions & 1 deletion docs/java-rest/usage.asciidoc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
28 changes: 2 additions & 26 deletions test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -101,31 +99,12 @@ public ClientYamlTestResponse callApi(String apiName, Map<String, String> params
}
}

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 {
Expand Down Expand Up @@ -197,9 +176,6 @@ public ClientYamlTestResponse callApi(String apiName, Map<String, String> 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);
}
}
Expand Down

0 comments on commit e79b03b

Please sign in to comment.