Extract constant for ?ignore pseudo-parameter (#102365)

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.
This commit is contained in:
David Turner 2023-11-20 08:37:02 +00:00 committed by GitHub
parent ce700b6f27
commit e8c3a72785
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
10 changed files with 41 additions and 18 deletions

View file

@ -87,6 +87,7 @@ import javax.net.ssl.SSLHandshakeException;
import static java.nio.charset.StandardCharsets.UTF_8; import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.Collections.singletonList; 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. * Client that connects to an Elasticsearch cluster through HTTP.
@ -106,6 +107,9 @@ import static java.util.Collections.singletonList;
* Requests can be traced by enabling trace logging for "tracer". The trace logger outputs requests and responses in curl format. * 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 class RestClient implements Closeable {
public static final String IGNORE_RESPONSE_CODES_PARAM = "ignore";
private static final Log logger = LogFactory.getLog(RestClient.class); private static final Log logger = LogFactory.getLog(RestClient.class);
private final CloseableHttpAsyncClient client; private final CloseableHttpAsyncClient client;
@ -780,8 +784,8 @@ public class RestClient implements Closeable {
this.request = request; this.request = request;
Map<String, String> params = new HashMap<>(request.getParameters()); Map<String, String> params = new HashMap<>(request.getParameters());
params.putAll(request.getOptions().getParameters()); params.putAll(request.getOptions().getParameters());
// ignore is a special parameter supported by the clients, shouldn't be sent to es // IGNORE_RESPONSE_CODES_PARAM is a special parameter supported by the clients, shouldn't be sent to es
String ignoreString = params.remove("ignore"); String ignoreString = params.remove(IGNORE_RESPONSE_CODES_PARAM);
this.ignoreErrorCodes = getIgnoreErrorCodes(ignoreString, request.getMethod()); this.ignoreErrorCodes = getIgnoreErrorCodes(ignoreString, request.getMethod());
URI uri = buildUri(pathPrefix, request.getEndpoint(), params); URI uri = buildUri(pathPrefix, request.getEndpoint(), params);
this.httpRequest = createHttpRequest(request.getMethod(), uri, request.getEntity(), compressionEnabled); this.httpRequest = createHttpRequest(request.getMethod(), uri, request.getEntity(), compressionEnabled);

View file

@ -275,6 +275,7 @@ public class RestClientSingleHostTests extends RestClientTestCase {
try { try {
Request request = new Request(method, "/" + errorStatusCode); Request request = new Request(method, "/" + errorStatusCode);
if (false == ignoreParam.isEmpty()) { 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); request.addParameter("ignore", ignoreParam);
} }
Response response = restClient.performRequest(request); Response response = restClient.performRequest(request);
@ -568,6 +569,7 @@ public class RestClientSingleHostTests extends RestClientTestCase {
if (randomBoolean()) { if (randomBoolean()) {
ignore += "," + randomFrom(RestClientTestUtil.getAllErrorStatusCodes()); 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); request.addParameter("ignore", ignore);
} }
URI uri = uriBuilder.build(); URI uri = uriBuilder.build();

View file

@ -112,6 +112,7 @@ import javax.net.ssl.SSLContext;
import static java.util.Collections.sort; import static java.util.Collections.sort;
import static java.util.Collections.unmodifiableList; 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.elasticsearch.core.Strings.format;
import static org.hamcrest.Matchers.anyOf; import static org.hamcrest.Matchers.anyOf;
import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.containsString;
@ -1157,7 +1158,7 @@ public abstract class ESRestTestCase extends ESTestCase {
@SuppressWarnings("unchecked") @SuppressWarnings("unchecked")
String jobId = (String) ((Map<String, Object>) jobConfig.get("config")).get("id"); String jobId = (String) ((Map<String, Object>) jobConfig.get("config")).get("id");
Request request = new Request("POST", "/_rollup/job/" + jobId + "/_stop"); 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("wait_for_completion", "true");
request.addParameter("timeout", "10s"); request.addParameter("timeout", "10s");
logger.debug("stopping rollup job [{}]", jobId); logger.debug("stopping rollup job [{}]", jobId);
@ -1168,7 +1169,7 @@ public abstract class ESRestTestCase extends ESTestCase {
@SuppressWarnings("unchecked") @SuppressWarnings("unchecked")
String jobId = (String) ((Map<String, Object>) jobConfig.get("config")).get("id"); String jobId = (String) ((Map<String, Object>) jobConfig.get("config")).get("id");
Request request = new Request("DELETE", "/_rollup/job/" + jobId); 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); logger.debug("deleting rollup job [{}]", jobId);
adminClient().performRequest(request); adminClient().performRequest(request);
} }
@ -1846,7 +1847,7 @@ public abstract class ESRestTestCase extends ESTestCase {
throws IOException { throws IOException {
final Request request = new Request(HttpDelete.METHOD_NAME, "_snapshot/" + repository + '/' + snapshot); final Request request = new Request(HttpDelete.METHOD_NAME, "_snapshot/" + repository + '/' + snapshot);
if (ignoreMissing) { if (ignoreMissing) {
request.addParameter("ignore", "404"); setIgnoredErrorResponseCodes(request, RestStatus.NOT_FOUND);
} }
final Response response = restClient.performRequest(request); final Response response = restClient.performRequest(request);
assertThat(response.getStatusLine().getStatusCode(), ignoreMissing ? anyOf(equalTo(200), equalTo(404)) : equalTo(200)); assertThat(response.getStatusLine().getStatusCode(), ignoreMissing ? anyOf(equalTo(200), equalTo(404)) : equalTo(200));
@ -2244,4 +2245,11 @@ public abstract class ESRestTestCase extends ESTestCase {
return historicalFeatures; 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(","))
);
}
} }

View file

@ -24,6 +24,8 @@ import java.util.Map;
import java.util.Set; import java.util.Set;
import java.util.stream.Stream; 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. * Holds the specification used to turn {@code do} actions in the YAML suite into REST api calls.
*/ */
@ -69,7 +71,7 @@ public class ClientYamlSuiteRestSpec {
* that they influence the client behaviour and don't get sent to Elasticsearch * that they influence the client behaviour and don't get sent to Elasticsearch
*/ */
public boolean isClientParameter(String name) { public boolean isClientParameter(String name) {
return "ignore".equals(name); return IGNORE_RESPONSE_CODES_PARAM.equals(name);
} }
/** /**

View file

@ -27,9 +27,11 @@ import java.io.IOException;
import java.util.Collections; import java.util.Collections;
import java.util.Map; import java.util.Map;
import java.util.Set; 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.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> * {@code PublishableHttpResource} represents an {@link HttpResource} that is a single file or object that can be checked <em>and</em>
@ -254,7 +256,7 @@ public abstract class PublishableHttpResource extends HttpResource {
// avoid exists and DNE parameters from being an exception by default // avoid exists and DNE parameters from being an exception by default
final Set<Integer> expectedResponseCodes = Sets.union(exists, doesNotExist); 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() { client.performRequestAsync(request, new ResponseListener() {
@ -436,9 +438,9 @@ public abstract class PublishableHttpResource extends HttpResource {
final Request request = new Request("DELETE", resourceBasePath + "/" + resourceName); final Request request = new Request("DELETE", resourceBasePath + "/" + resourceName);
addDefaultParameters(request); addDefaultParameters(request);
if (false == defaultParameters.containsKey("ignore")) { if (false == defaultParameters.containsKey(IGNORE_RESPONSE_CODES_PARAM)) {
// avoid 404 being an exception by default // 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() { client.performRequestAsync(request, new ResponseListener() {

View file

@ -30,8 +30,9 @@ import java.util.HashMap;
import java.util.Map; import java.util.Map;
import java.util.Set; import java.util.Set;
import java.util.function.Predicate; 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.mockBooleanActionListener;
import static org.elasticsearch.xpack.monitoring.exporter.http.AsyncHttpResourceHelper.mockPublishResultActionListener; import static org.elasticsearch.xpack.monitoring.exporter.http.AsyncHttpResourceHelper.mockPublishResultActionListener;
import static org.elasticsearch.xpack.monitoring.exporter.http.AsyncHttpResourceHelper.whenPerformRequestAsyncWith; import static org.elasticsearch.xpack.monitoring.exporter.http.AsyncHttpResourceHelper.whenPerformRequestAsyncWith;
@ -443,7 +444,7 @@ public abstract class AbstractPublishableHttpResourceTestCase extends ESTestCase
final Set<Integer> statusCodes = Sets.union(exists, doesNotExist); final Set<Integer> statusCodes = Sets.union(exists, doesNotExist);
final Map<String, String> parametersWithIgnore = new HashMap<>(parameters); 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; return parametersWithIgnore;
} }
@ -451,7 +452,7 @@ public abstract class AbstractPublishableHttpResourceTestCase extends ESTestCase
protected Map<String, String> deleteParameters(final Map<String, String> parameters) { protected Map<String, String> deleteParameters(final Map<String, String> parameters) {
final Map<String, String> parametersWithIgnore = new HashMap<>(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; return parametersWithIgnore;
} }

View file

@ -25,6 +25,7 @@ import org.elasticsearch.common.settings.SecureString;
import org.elasticsearch.common.xcontent.XContentParserUtils; import org.elasticsearch.common.xcontent.XContentParserUtils;
import org.elasticsearch.core.Nullable; import org.elasticsearch.core.Nullable;
import org.elasticsearch.core.Tuple; import org.elasticsearch.core.Tuple;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.xcontent.ObjectPath; import org.elasticsearch.xcontent.ObjectPath;
import org.elasticsearch.xcontent.ToXContent; import org.elasticsearch.xcontent.ToXContent;
import org.elasticsearch.xcontent.XContentBuilder; import org.elasticsearch.xcontent.XContentBuilder;
@ -51,6 +52,7 @@ import java.util.stream.Collectors;
import static org.elasticsearch.common.xcontent.XContentParserUtils.ensureExpectedToken; import static org.elasticsearch.common.xcontent.XContentParserUtils.ensureExpectedToken;
import static org.elasticsearch.test.rest.ESRestTestCase.entityAsMap; import static org.elasticsearch.test.rest.ESRestTestCase.entityAsMap;
import static org.elasticsearch.test.rest.ESRestTestCase.setIgnoredErrorResponseCodes;
public class TestSecurityClient { public class TestSecurityClient {
@ -395,7 +397,7 @@ public class TestSecurityClient {
final Request request = new Request(HttpDelete.METHOD_NAME, endpoint); 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. // 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 // 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); request.setJsonEntity(requestBody);
final Map<String, Object> responseBody = entityAsMap(execute(request)); final Map<String, Object> responseBody = entityAsMap(execute(request));
final List<Map<String, ?>> errors = (List<Map<String, ?>>) responseBody.get("error_details"); final List<Map<String, ?>> errors = (List<Map<String, ?>>) responseBody.get("error_details");

View file

@ -19,6 +19,7 @@ import org.elasticsearch.common.settings.SecureString;
import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.concurrent.ThreadContext; import org.elasticsearch.common.util.concurrent.ThreadContext;
import org.elasticsearch.common.xcontent.support.XContentMapValues; import org.elasticsearch.common.xcontent.support.XContentMapValues;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.test.rest.ESRestTestCase; import org.elasticsearch.test.rest.ESRestTestCase;
import org.elasticsearch.xcontent.XContentBuilder; import org.elasticsearch.xcontent.XContentBuilder;
import org.elasticsearch.xpack.core.transform.TransformField; import org.elasticsearch.xpack.core.transform.TransformField;
@ -618,7 +619,7 @@ public abstract class TransformRestTestCase extends ESRestTestCase {
protected static void deleteTransform(String transformId, boolean ignoreNotFound, boolean deleteDestIndex) throws IOException { protected static void deleteTransform(String transformId, boolean ignoreNotFound, boolean deleteDestIndex) throws IOException {
Request request = new Request("DELETE", getTransformEndpoint() + transformId); Request request = new Request("DELETE", getTransformEndpoint() + transformId);
if (ignoreNotFound) { if (ignoreNotFound) {
request.addParameter("ignore", "404"); setIgnoredErrorResponseCodes(request, RestStatus.NOT_FOUND);
} }
if (deleteDestIndex) { if (deleteDestIndex) {
request.addParameter(TransformField.DELETE_DEST_INDEX.getPreferredName(), Boolean.TRUE.toString()); request.addParameter(TransformField.DELETE_DEST_INDEX.getPreferredName(), Boolean.TRUE.toString());

View file

@ -8,6 +8,7 @@ package org.elasticsearch.upgrades;
import org.elasticsearch.client.Request; import org.elasticsearch.client.Request;
import org.elasticsearch.client.Response; import org.elasticsearch.client.Response;
import org.elasticsearch.rest.RestStatus;
import java.util.Map; import java.util.Map;
@ -28,7 +29,7 @@ public class BasicLicenseUpgradeIT extends AbstractUpgradeTestCase {
final Request request = new Request("GET", "/_license"); final Request request = new Request("GET", "/_license");
// This avoids throwing a ResponseException when the license is not ready yet // This avoids throwing a ResponseException when the license is not ready yet
// allowing to retry the check using assertBusy // allowing to retry the check using assertBusy
request.addParameter("ignore", "404"); setIgnoredErrorResponseCodes(request, RestStatus.NOT_FOUND);
Response licenseResponse = client().performRequest(request); Response licenseResponse = client().performRequest(request);
assertOK(licenseResponse); assertOK(licenseResponse);
Map<String, Object> licenseResponseMap = entityAsMap(licenseResponse); Map<String, Object> licenseResponseMap = entityAsMap(licenseResponse);
@ -42,7 +43,7 @@ public class BasicLicenseUpgradeIT extends AbstractUpgradeTestCase {
final Request request = new Request("GET", "/_license"); final Request request = new Request("GET", "/_license");
// This avoids throwing a ResponseException when the license is not ready yet // This avoids throwing a ResponseException when the license is not ready yet
// allowing to retry the check using assertBusy // allowing to retry the check using assertBusy
request.addParameter("ignore", "404"); setIgnoredErrorResponseCodes(request, RestStatus.NOT_FOUND);
Response licenseResponse = client().performRequest(request); Response licenseResponse = client().performRequest(request);
assertOK(licenseResponse); assertOK(licenseResponse);
Map<String, Object> licenseResponseMap = entityAsMap(licenseResponse); Map<String, Object> licenseResponseMap = entityAsMap(licenseResponse);

View file

@ -371,7 +371,7 @@ public class CcrRollingUpgradeIT extends AbstractMultiClusterUpgradeTestCase {
private static void verifyTotalHitCount(final String index, final int expectedTotalHits, final RestClient client) throws IOException { private static void verifyTotalHitCount(final String index, final int expectedTotalHits, final RestClient client) throws IOException {
final Request request = new Request("GET", "/" + index + "/_search"); final Request request = new Request("GET", "/" + index + "/_search");
request.addParameter(TOTAL_HITS_AS_INT_PARAM, "true"); 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))); Map<?, ?> response = toMap(assertOK(client.performRequest(request)));
final int totalHits = (int) XContentMapValues.extractValue("hits.total", response); final int totalHits = (int) XContentMapValues.extractValue("hits.total", response);
assertThat(totalHits, equalTo(expectedTotalHits)); assertThat(totalHits, equalTo(expectedTotalHits));