Skip to content

Commit

Permalink
Extract constant for ?ignore pseudo-parameter (elastic#102365)
Browse files Browse the repository at this point in the history
Today `RestClient` interprets an `?ignore=` request parameter as an
indication that certain HTTP response codes should be considered
successful and not raise a `ResponseException`. This commit replaces the
magic literal `"ignore"` with a constant and adds a utility to specify
the ignored codes as `RestStatus` values.
  • Loading branch information
DaveCTurner authored Nov 20, 2023
1 parent ce700b6 commit e8c3a72
Show file tree
Hide file tree
Showing 10 changed files with 41 additions and 18 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@

import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.Collections.singletonList;
import static org.elasticsearch.client.RestClient.IGNORE_RESPONSE_CODES_PARAM;

/**
* Client that connects to an Elasticsearch cluster through HTTP.
Expand All @@ -106,6 +107,9 @@
* Requests can be traced by enabling trace logging for "tracer". The trace logger outputs requests and responses in curl format.
*/
public class RestClient implements Closeable {

public static final String IGNORE_RESPONSE_CODES_PARAM = "ignore";

private static final Log logger = LogFactory.getLog(RestClient.class);

private final CloseableHttpAsyncClient client;
Expand Down Expand Up @@ -780,8 +784,8 @@ private class InternalRequest {
this.request = request;
Map<String, String> params = new HashMap<>(request.getParameters());
params.putAll(request.getOptions().getParameters());
// ignore is a special parameter supported by the clients, shouldn't be sent to es
String ignoreString = params.remove("ignore");
// IGNORE_RESPONSE_CODES_PARAM is a special parameter supported by the clients, shouldn't be sent to es
String ignoreString = params.remove(IGNORE_RESPONSE_CODES_PARAM);
this.ignoreErrorCodes = getIgnoreErrorCodes(ignoreString, request.getMethod());
URI uri = buildUri(pathPrefix, request.getEndpoint(), params);
this.httpRequest = createHttpRequest(request.getMethod(), uri, request.getEntity(), compressionEnabled);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,7 @@ public void testErrorStatusCodes() throws Exception {
try {
Request request = new Request(method, "/" + errorStatusCode);
if (false == ignoreParam.isEmpty()) {
// literal "ignore" rather than IGNORE_RESPONSE_CODES_PARAM since this is something on which callers might rely
request.addParameter("ignore", ignoreParam);
}
Response response = restClient.performRequest(request);
Expand Down Expand Up @@ -568,6 +569,7 @@ private HttpUriRequest performRandomRequest(String method) throws Exception {
if (randomBoolean()) {
ignore += "," + randomFrom(RestClientTestUtil.getAllErrorStatusCodes());
}
// literal "ignore" rather than IGNORE_RESPONSE_CODES_PARAM since this is something on which callers might rely
request.addParameter("ignore", ignore);
}
URI uri = uriBuilder.build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@

import static java.util.Collections.sort;
import static java.util.Collections.unmodifiableList;
import static org.elasticsearch.client.RestClient.IGNORE_RESPONSE_CODES_PARAM;
import static org.elasticsearch.core.Strings.format;
import static org.hamcrest.Matchers.anyOf;
import static org.hamcrest.Matchers.containsString;
Expand Down Expand Up @@ -1157,7 +1158,7 @@ private void wipeRollupJobs() throws IOException {
@SuppressWarnings("unchecked")
String jobId = (String) ((Map<String, Object>) jobConfig.get("config")).get("id");
Request request = new Request("POST", "/_rollup/job/" + jobId + "/_stop");
request.addParameter("ignore", "404");
setIgnoredErrorResponseCodes(request, RestStatus.NOT_FOUND);
request.addParameter("wait_for_completion", "true");
request.addParameter("timeout", "10s");
logger.debug("stopping rollup job [{}]", jobId);
Expand All @@ -1168,7 +1169,7 @@ private void wipeRollupJobs() throws IOException {
@SuppressWarnings("unchecked")
String jobId = (String) ((Map<String, Object>) jobConfig.get("config")).get("id");
Request request = new Request("DELETE", "/_rollup/job/" + jobId);
request.addParameter("ignore", "404"); // Ignore 404s because they imply someone was racing us to delete this
setIgnoredErrorResponseCodes(request, RestStatus.NOT_FOUND); // 404s imply someone was racing us to delete this
logger.debug("deleting rollup job [{}]", jobId);
adminClient().performRequest(request);
}
Expand Down Expand Up @@ -1846,7 +1847,7 @@ protected static void deleteSnapshot(RestClient restClient, String repository, S
throws IOException {
final Request request = new Request(HttpDelete.METHOD_NAME, "_snapshot/" + repository + '/' + snapshot);
if (ignoreMissing) {
request.addParameter("ignore", "404");
setIgnoredErrorResponseCodes(request, RestStatus.NOT_FOUND);
}
final Response response = restClient.performRequest(request);
assertThat(response.getStatusLine().getStatusCode(), ignoreMissing ? anyOf(equalTo(200), equalTo(404)) : equalTo(200));
Expand Down Expand Up @@ -2244,4 +2245,11 @@ protected Map<NodeFeature, Version> getHistoricalFeatures() {

return historicalFeatures;
}

public static void setIgnoredErrorResponseCodes(Request request, RestStatus... restStatuses) {
request.addParameter(
IGNORE_RESPONSE_CODES_PARAM,
Arrays.stream(restStatuses).map(restStatus -> Integer.toString(restStatus.getStatus())).collect(Collectors.joining(","))
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
import java.util.Set;
import java.util.stream.Stream;

import static org.elasticsearch.client.RestClient.IGNORE_RESPONSE_CODES_PARAM;

/**
* Holds the specification used to turn {@code do} actions in the YAML suite into REST api calls.
*/
Expand Down Expand Up @@ -69,7 +71,7 @@ public boolean isGlobalParameter(String param) {
* that they influence the client behaviour and don't get sent to Elasticsearch
*/
public boolean isClientParameter(String name) {
return "ignore".equals(name);
return IGNORE_RESPONSE_CODES_PARAM.equals(name);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,11 @@
import java.util.Collections;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;

import static java.util.stream.Collectors.joining;
import static org.elasticsearch.client.RestClient.IGNORE_RESPONSE_CODES_PARAM;
import static org.elasticsearch.core.Strings.format;
import static org.elasticsearch.rest.RestStatus.NOT_FOUND;

/**
* {@code PublishableHttpResource} represents an {@link HttpResource} that is a single file or object that can be checked <em>and</em>
Expand Down Expand Up @@ -254,7 +256,7 @@ protected void checkForResource(

// avoid exists and DNE parameters from being an exception by default
final Set<Integer> expectedResponseCodes = Sets.union(exists, doesNotExist);
request.addParameter("ignore", expectedResponseCodes.stream().map(i -> i.toString()).collect(Collectors.joining(",")));
request.addParameter(IGNORE_RESPONSE_CODES_PARAM, expectedResponseCodes.stream().map(Object::toString).collect(joining(",")));

client.performRequestAsync(request, new ResponseListener() {

Expand Down Expand Up @@ -436,9 +438,9 @@ protected void deleteResource(
final Request request = new Request("DELETE", resourceBasePath + "/" + resourceName);
addDefaultParameters(request);

if (false == defaultParameters.containsKey("ignore")) {
if (false == defaultParameters.containsKey(IGNORE_RESPONSE_CODES_PARAM)) {
// avoid 404 being an exception by default
request.addParameter("ignore", Integer.toString(RestStatus.NOT_FOUND.getStatus()));
request.addParameter(IGNORE_RESPONSE_CODES_PARAM, Integer.toString(NOT_FOUND.getStatus()));
}

client.performRequestAsync(request, new ResponseListener() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,9 @@
import java.util.Map;
import java.util.Set;
import java.util.function.Predicate;
import java.util.stream.Collectors;

import static java.util.stream.Collectors.joining;
import static org.elasticsearch.client.RestClient.IGNORE_RESPONSE_CODES_PARAM;
import static org.elasticsearch.xpack.monitoring.exporter.http.AsyncHttpResourceHelper.mockBooleanActionListener;
import static org.elasticsearch.xpack.monitoring.exporter.http.AsyncHttpResourceHelper.mockPublishResultActionListener;
import static org.elasticsearch.xpack.monitoring.exporter.http.AsyncHttpResourceHelper.whenPerformRequestAsyncWith;
Expand Down Expand Up @@ -443,15 +444,15 @@ protected Map<String, String> getParameters(
final Set<Integer> statusCodes = Sets.union(exists, doesNotExist);
final Map<String, String> parametersWithIgnore = new HashMap<>(parameters);

parametersWithIgnore.putIfAbsent("ignore", statusCodes.stream().map(i -> i.toString()).collect(Collectors.joining(",")));
parametersWithIgnore.putIfAbsent(IGNORE_RESPONSE_CODES_PARAM, statusCodes.stream().map(Object::toString).collect(joining(",")));

return parametersWithIgnore;
}

protected Map<String, String> deleteParameters(final Map<String, String> parameters) {
final Map<String, String> parametersWithIgnore = new HashMap<>(parameters);

parametersWithIgnore.putIfAbsent("ignore", "404");
parametersWithIgnore.putIfAbsent(IGNORE_RESPONSE_CODES_PARAM, Integer.toString(RestStatus.NOT_FOUND.getStatus()));

return parametersWithIgnore;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.elasticsearch.common.xcontent.XContentParserUtils;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.core.Tuple;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.xcontent.ObjectPath;
import org.elasticsearch.xcontent.ToXContent;
import org.elasticsearch.xcontent.XContentBuilder;
Expand All @@ -51,6 +52,7 @@

import static org.elasticsearch.common.xcontent.XContentParserUtils.ensureExpectedToken;
import static org.elasticsearch.test.rest.ESRestTestCase.entityAsMap;
import static org.elasticsearch.test.rest.ESRestTestCase.setIgnoredErrorResponseCodes;

public class TestSecurityClient {

Expand Down Expand Up @@ -395,7 +397,7 @@ public TokenInvalidation invalidateTokens(String requestBody) throws IOException
final Request request = new Request(HttpDelete.METHOD_NAME, endpoint);
// This API returns 404 (with the same body as a 200 response) if there's nothing to delete.
// RestClient will throw an exception on 404, but we don't want that, we want to parse the body and return it
request.addParameter("ignore", "404");
setIgnoredErrorResponseCodes(request, RestStatus.NOT_FOUND);
request.setJsonEntity(requestBody);
final Map<String, Object> responseBody = entityAsMap(execute(request));
final List<Map<String, ?>> errors = (List<Map<String, ?>>) responseBody.get("error_details");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.concurrent.ThreadContext;
import org.elasticsearch.common.xcontent.support.XContentMapValues;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.test.rest.ESRestTestCase;
import org.elasticsearch.xcontent.XContentBuilder;
import org.elasticsearch.xpack.core.transform.TransformField;
Expand Down Expand Up @@ -618,7 +619,7 @@ protected static void deleteTransform(String transformId) throws IOException {
protected static void deleteTransform(String transformId, boolean ignoreNotFound, boolean deleteDestIndex) throws IOException {
Request request = new Request("DELETE", getTransformEndpoint() + transformId);
if (ignoreNotFound) {
request.addParameter("ignore", "404");
setIgnoredErrorResponseCodes(request, RestStatus.NOT_FOUND);
}
if (deleteDestIndex) {
request.addParameter(TransformField.DELETE_DEST_INDEX.getPreferredName(), Boolean.TRUE.toString());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import org.elasticsearch.client.Request;
import org.elasticsearch.client.Response;
import org.elasticsearch.rest.RestStatus;

import java.util.Map;

Expand All @@ -28,7 +29,7 @@ private void checkBasicLicense() throws Exception {
final Request request = new Request("GET", "/_license");
// This avoids throwing a ResponseException when the license is not ready yet
// allowing to retry the check using assertBusy
request.addParameter("ignore", "404");
setIgnoredErrorResponseCodes(request, RestStatus.NOT_FOUND);
Response licenseResponse = client().performRequest(request);
assertOK(licenseResponse);
Map<String, Object> licenseResponseMap = entityAsMap(licenseResponse);
Expand All @@ -42,7 +43,7 @@ private void checkNonExpiringBasicLicense() throws Exception {
final Request request = new Request("GET", "/_license");
// This avoids throwing a ResponseException when the license is not ready yet
// allowing to retry the check using assertBusy
request.addParameter("ignore", "404");
setIgnoredErrorResponseCodes(request, RestStatus.NOT_FOUND);
Response licenseResponse = client().performRequest(request);
assertOK(licenseResponse);
Map<String, Object> licenseResponseMap = entityAsMap(licenseResponse);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ private static void assertTotalHitCount(final String index, final int expectedTo
private static void verifyTotalHitCount(final String index, final int expectedTotalHits, final RestClient client) throws IOException {
final Request request = new Request("GET", "/" + index + "/_search");
request.addParameter(TOTAL_HITS_AS_INT_PARAM, "true");
request.addParameter("ignore", "404"); // If index not found, trip the assertOK (i.e. retry an assertBusy) rather than throwing
setIgnoredErrorResponseCodes(request, RestStatus.NOT_FOUND); // trip the assertOK (i.e. retry an assertBusy) rather than throwing
Map<?, ?> response = toMap(assertOK(client.performRequest(request)));
final int totalHits = (int) XContentMapValues.extractValue("hits.total", response);
assertThat(totalHits, equalTo(expectedTotalHits));
Expand Down

0 comments on commit e8c3a72

Please sign in to comment.