From e8c3a72785a76159d8e341f04d9e173e4e8c6f85 Mon Sep 17 00:00:00 2001 From: David Turner Date: Mon, 20 Nov 2023 08:37:02 +0000 Subject: [PATCH] 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. --- .../java/org/elasticsearch/client/RestClient.java | 8 ++++++-- .../client/RestClientSingleHostTests.java | 2 ++ .../elasticsearch/test/rest/ESRestTestCase.java | 14 +++++++++++--- .../yaml/restspec/ClientYamlSuiteRestSpec.java | 4 +++- .../exporter/http/PublishableHttpResource.java | 10 ++++++---- .../AbstractPublishableHttpResourceTestCase.java | 7 ++++--- .../org/elasticsearch/test/TestSecurityClient.java | 4 +++- .../integration/TransformRestTestCase.java | 3 ++- .../upgrades/BasicLicenseUpgradeIT.java | 5 +++-- .../upgrades/CcrRollingUpgradeIT.java | 2 +- 10 files changed, 41 insertions(+), 18 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 7154a2be5bbd8..ed087bef0ac76 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/RestClient.java +++ b/client/rest/src/main/java/org/elasticsearch/client/RestClient.java @@ -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. @@ -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; @@ -780,8 +784,8 @@ private class InternalRequest { this.request = request; Map 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); 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 a1c4d3fab076a..10d24242ae620 100644 --- a/client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostTests.java +++ b/client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostTests.java @@ -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); @@ -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(); diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java index a3ff4d87da73e..3327137cef7b7 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java @@ -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; @@ -1157,7 +1158,7 @@ private void wipeRollupJobs() throws IOException { @SuppressWarnings("unchecked") String jobId = (String) ((Map) 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); @@ -1168,7 +1169,7 @@ private void wipeRollupJobs() throws IOException { @SuppressWarnings("unchecked") String jobId = (String) ((Map) 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); } @@ -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)); @@ -2244,4 +2245,11 @@ protected Map 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(",")) + ); + } } diff --git a/test/yaml-rest-runner/src/main/java/org/elasticsearch/test/rest/yaml/restspec/ClientYamlSuiteRestSpec.java b/test/yaml-rest-runner/src/main/java/org/elasticsearch/test/rest/yaml/restspec/ClientYamlSuiteRestSpec.java index be34ee9be0ea1..8662d886cce89 100644 --- a/test/yaml-rest-runner/src/main/java/org/elasticsearch/test/rest/yaml/restspec/ClientYamlSuiteRestSpec.java +++ b/test/yaml-rest-runner/src/main/java/org/elasticsearch/test/rest/yaml/restspec/ClientYamlSuiteRestSpec.java @@ -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. */ @@ -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); } /** diff --git a/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/PublishableHttpResource.java b/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/PublishableHttpResource.java index d37f4669484a0..e2d4d173af013 100644 --- a/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/PublishableHttpResource.java +++ b/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/PublishableHttpResource.java @@ -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 and @@ -254,7 +256,7 @@ protected void checkForResource( // avoid exists and DNE parameters from being an exception by default final Set 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() { @@ -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() { diff --git a/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/AbstractPublishableHttpResourceTestCase.java b/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/AbstractPublishableHttpResourceTestCase.java index 4878289cae8d6..b72891708e780 100644 --- a/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/AbstractPublishableHttpResourceTestCase.java +++ b/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/AbstractPublishableHttpResourceTestCase.java @@ -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; @@ -443,7 +444,7 @@ protected Map getParameters( final Set statusCodes = Sets.union(exists, doesNotExist); final Map 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; } @@ -451,7 +452,7 @@ protected Map getParameters( protected Map deleteParameters(final Map parameters) { final Map parametersWithIgnore = new HashMap<>(parameters); - parametersWithIgnore.putIfAbsent("ignore", "404"); + parametersWithIgnore.putIfAbsent(IGNORE_RESPONSE_CODES_PARAM, Integer.toString(RestStatus.NOT_FOUND.getStatus())); return parametersWithIgnore; } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/test/TestSecurityClient.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/test/TestSecurityClient.java index 13c8612487d89..4888c0f4c9721 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/test/TestSecurityClient.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/test/TestSecurityClient.java @@ -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; @@ -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 { @@ -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 responseBody = entityAsMap(execute(request)); final List> errors = (List>) responseBody.get("error_details"); diff --git a/x-pack/plugin/transform/qa/single-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/transform/integration/TransformRestTestCase.java b/x-pack/plugin/transform/qa/single-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/transform/integration/TransformRestTestCase.java index e6388bb6fea5d..c616c1c238171 100644 --- a/x-pack/plugin/transform/qa/single-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/transform/integration/TransformRestTestCase.java +++ b/x-pack/plugin/transform/qa/single-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/transform/integration/TransformRestTestCase.java @@ -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; @@ -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()); diff --git a/x-pack/qa/rolling-upgrade-basic/src/test/java/org/elasticsearch/upgrades/BasicLicenseUpgradeIT.java b/x-pack/qa/rolling-upgrade-basic/src/test/java/org/elasticsearch/upgrades/BasicLicenseUpgradeIT.java index 75fcc5cf6e7ad..da8a4c806a0f5 100644 --- a/x-pack/qa/rolling-upgrade-basic/src/test/java/org/elasticsearch/upgrades/BasicLicenseUpgradeIT.java +++ b/x-pack/qa/rolling-upgrade-basic/src/test/java/org/elasticsearch/upgrades/BasicLicenseUpgradeIT.java @@ -8,6 +8,7 @@ import org.elasticsearch.client.Request; import org.elasticsearch.client.Response; +import org.elasticsearch.rest.RestStatus; import java.util.Map; @@ -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 licenseResponseMap = entityAsMap(licenseResponse); @@ -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 licenseResponseMap = entityAsMap(licenseResponse); diff --git a/x-pack/qa/rolling-upgrade-multi-cluster/src/test/java/org/elasticsearch/upgrades/CcrRollingUpgradeIT.java b/x-pack/qa/rolling-upgrade-multi-cluster/src/test/java/org/elasticsearch/upgrades/CcrRollingUpgradeIT.java index 59928470eb247..0b7ab1fe5980d 100644 --- a/x-pack/qa/rolling-upgrade-multi-cluster/src/test/java/org/elasticsearch/upgrades/CcrRollingUpgradeIT.java +++ b/x-pack/qa/rolling-upgrade-multi-cluster/src/test/java/org/elasticsearch/upgrades/CcrRollingUpgradeIT.java @@ -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));