From 47f45cb74f1d215f42c5f1350718d95cd18f114a Mon Sep 17 00:00:00 2001 From: lipsill Date: Fri, 14 Sep 2018 11:56:54 +0200 Subject: [PATCH 1/3] REST client: introduce a strict mode `es.deprecation.mode.strict` Minimal change that marks a rest request as failed if a deprecation warning has been issued by the server. This is valuable for internal tests and documentation snippets so that only non-deprecated code is used. This also minimizes possible divergence of the high level REST client from the server. `off` (default): the warning is logged at WARN level. As before the response is returned to the caller for further analysis. The caller still can retrieve all warnings from the response. `on`: the warning is logged at WARN level but the response is marked as failed. The caller can retrieve all warnings from the ResponseException. --- .../org/elasticsearch/client/Response.java | 38 +++++++++++++++++++ .../client/ResponseException.java | 4 ++ .../org/elasticsearch/client/RestClient.java | 8 +++- 3 files changed, 49 insertions(+), 1 deletion(-) diff --git a/client/rest/src/main/java/org/elasticsearch/client/Response.java b/client/rest/src/main/java/org/elasticsearch/client/Response.java index 39bbf769713b2..71309f9f0e88b 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/Response.java +++ b/client/rest/src/main/java/org/elasticsearch/client/Response.java @@ -26,7 +26,11 @@ import org.apache.http.RequestLine; import org.apache.http.StatusLine; +import java.util.ArrayList; +import java.util.List; import java.util.Objects; +import java.util.regex.Matcher; +import java.util.regex.Pattern; /** * Holds an elasticsearch response. It wraps the {@link HttpResponse} returned and associates it with @@ -96,6 +100,40 @@ public HttpEntity getEntity() { return response.getEntity(); } + private static Pattern WARNING_HEADER_PATTERN = Pattern.compile( + "299 " + // warn code + "Elasticsearch-\\d+\\.\\d+\\.\\d+(?:-(?:alpha|beta|rc)\\d+)?(?:-SNAPSHOT)?-(?:[a-f0-9]{7}|Unknown) " + // warn agent + "\"((?:\t| |!|[\\x23-\\x5B]|[\\x5D-\\x7E]|[\\x80-\\xFF]|\\\\|\\\\\")*)\" " + // quoted warning value, captured + // quoted RFC 1123 date format + "\"" + // opening quote + "(?:Mon|Tue|Wed|Thu|Fri|Sat|Sun), " + // weekday + "\\d{2} " + // 2-digit day + "(?:Jan|Feb|Mar|Apr|May|Jun|Jul|Aug|Sep|Oct|Nov|Dec) " + // month + "\\d{4} " + // 4-digit year + "\\d{2}:\\d{2}:\\d{2} " + // (two-digit hour):(two-digit minute):(two-digit second) + "GMT" + // GMT + "\""); // closing quote + + + public List getWarnings() { + List warnings = new ArrayList<>(); + for (Header header : response.getHeaders("Warning")) { + String warning = header.getValue(); + final Matcher matcher = WARNING_HEADER_PATTERN.matcher(warning); + if (matcher.matches()) { + warnings.add(matcher.group(1)); + continue; + } + warnings.add(warning); + } + return warnings; + } + + public boolean hasWarnings() { + Header[] warnings = response.getHeaders("Warning"); + return warnings != null && warnings.length > 0; + } + HttpResponse getHttpResponse() { return response; } diff --git a/client/rest/src/main/java/org/elasticsearch/client/ResponseException.java b/client/rest/src/main/java/org/elasticsearch/client/ResponseException.java index 5e646d975c89c..0957e25fb7033 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/ResponseException.java +++ b/client/rest/src/main/java/org/elasticsearch/client/ResponseException.java @@ -58,6 +58,10 @@ private static String buildMessage(Response response) throws IOException { response.getStatusLine().toString() ); + if (response.hasWarnings()) { + message += "\n" + "Warnings: " + response.getWarnings(); + } + HttpEntity entity = response.getEntity(); if (entity != null) { if (entity.isRepeatable() == false) { 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 a7afbc8ffbd6d..ac7a9c43ceb2f 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/RestClient.java +++ b/client/rest/src/main/java/org/elasticsearch/client/RestClient.java @@ -110,6 +110,7 @@ public class RestClient implements Closeable { private final FailureListener failureListener; private final NodeSelector nodeSelector; private volatile NodeTuple> nodeTuple; + private final boolean strictDeprecationMode; RestClient(CloseableHttpAsyncClient client, long maxRetryTimeoutMillis, Header[] defaultHeaders, List nodes, String pathPrefix, FailureListener failureListener, NodeSelector nodeSelector) { @@ -119,6 +120,7 @@ public class RestClient implements Closeable { this.failureListener = failureListener; this.pathPrefix = pathPrefix; this.nodeSelector = nodeSelector; + this.strictDeprecationMode = System.getProperty("es.deprecation.mode.strict", "off").equals("on"); setNodes(nodes); } @@ -296,7 +298,11 @@ public void completed(HttpResponse httpResponse) { Response response = new Response(request.getRequestLine(), node.getHost(), httpResponse); if (isSuccessfulResponse(statusCode) || ignoreErrorCodes.contains(response.getStatusLine().getStatusCode())) { onResponse(node); - listener.onSuccess(response); + if (strictDeprecationMode && response.hasWarnings()) { + listener.onDefinitiveFailure(new ResponseException(response)); + } else { + listener.onSuccess(response); + } } else { ResponseException responseException = new ResponseException(response); if (isRetryStatus(statusCode)) { From ef1c4ea5a3bfe01a1ccf20cfbe303a5fb3a210e5 Mon Sep 17 00:00:00 2001 From: lipsill Date: Tue, 25 Sep 2018 19:05:23 +0200 Subject: [PATCH 2/3] Use ctor param instead of a system property to specify strict deprecation mode for the REST client --- .../java/org/elasticsearch/client/RestClient.java | 6 +++--- .../org/elasticsearch/client/RestClientBuilder.java | 12 +++++++++++- .../client/RestClientMultipleHostsTests.java | 2 +- .../client/RestClientSingleHostTests.java | 2 +- .../org/elasticsearch/client/RestClientTests.java | 4 ++-- .../org/elasticsearch/test/rest/ESRestTestCase.java | 4 ++++ 6 files changed, 22 insertions(+), 8 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 ac7a9c43ceb2f..d68e371f31836 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/RestClient.java +++ b/client/rest/src/main/java/org/elasticsearch/client/RestClient.java @@ -112,15 +112,15 @@ public class RestClient implements Closeable { private volatile NodeTuple> nodeTuple; private final boolean strictDeprecationMode; - RestClient(CloseableHttpAsyncClient client, long maxRetryTimeoutMillis, Header[] defaultHeaders, - List nodes, String pathPrefix, FailureListener failureListener, NodeSelector nodeSelector) { + RestClient(CloseableHttpAsyncClient client, long maxRetryTimeoutMillis, Header[] defaultHeaders, List nodes, String pathPrefix, + FailureListener failureListener, NodeSelector nodeSelector, boolean strictDeprecationMode) { this.client = client; this.maxRetryTimeoutMillis = maxRetryTimeoutMillis; this.defaultHeaders = Collections.unmodifiableList(Arrays.asList(defaultHeaders)); this.failureListener = failureListener; this.pathPrefix = pathPrefix; this.nodeSelector = nodeSelector; - this.strictDeprecationMode = System.getProperty("es.deprecation.mode.strict", "off").equals("on"); + this.strictDeprecationMode = strictDeprecationMode; setNodes(nodes); } diff --git a/client/rest/src/main/java/org/elasticsearch/client/RestClientBuilder.java b/client/rest/src/main/java/org/elasticsearch/client/RestClientBuilder.java index dd3f5ad5a7274..84cc3ee1667b1 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/RestClientBuilder.java +++ b/client/rest/src/main/java/org/elasticsearch/client/RestClientBuilder.java @@ -56,6 +56,7 @@ public final class RestClientBuilder { private RequestConfigCallback requestConfigCallback; private String pathPrefix; private NodeSelector nodeSelector = NodeSelector.ANY; + private boolean strictDeprecationMode = false; /** * Creates a new builder instance and sets the hosts that the client will send requests to. @@ -185,6 +186,15 @@ public RestClientBuilder setNodeSelector(NodeSelector nodeSelector) { return this; } + /** + * Whether the REST client should return any response containing at least + * one warning header as a failure. + */ + public RestClientBuilder setStrictDeprecationMode(boolean strictDeprecationMode) { + this.strictDeprecationMode = strictDeprecationMode; + return this; + } + /** * Creates a new {@link RestClient} based on the provided configuration. */ @@ -199,7 +209,7 @@ public CloseableHttpAsyncClient run() { } }); RestClient restClient = new RestClient(httpClient, maxRetryTimeout, defaultHeaders, nodes, - pathPrefix, failureListener, nodeSelector); + pathPrefix, failureListener, nodeSelector, strictDeprecationMode); httpClient.start(); return restClient; } diff --git a/client/rest/src/test/java/org/elasticsearch/client/RestClientMultipleHostsTests.java b/client/rest/src/test/java/org/elasticsearch/client/RestClientMultipleHostsTests.java index e1062076a0dbf..7dd1c4d842bff 100644 --- a/client/rest/src/test/java/org/elasticsearch/client/RestClientMultipleHostsTests.java +++ b/client/rest/src/test/java/org/elasticsearch/client/RestClientMultipleHostsTests.java @@ -115,7 +115,7 @@ public void run() { } nodes = Collections.unmodifiableList(nodes); failureListener = new HostsTrackingFailureListener(); - return new RestClient(httpClient, 10000, new Header[0], nodes, null, failureListener, nodeSelector); + return new RestClient(httpClient, 10000, new Header[0], nodes, null, failureListener, nodeSelector, false); } /** 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 0c589e6a40c8a..3aa1076267673 100644 --- a/client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostTests.java +++ b/client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostTests.java @@ -148,7 +148,7 @@ public void run() { node = new Node(new HttpHost("localhost", 9200)); failureListener = new HostsTrackingFailureListener(); restClient = new RestClient(httpClient, 10000, defaultHeaders, - singletonList(node), null, failureListener, NodeSelector.ANY); + singletonList(node), null, failureListener, NodeSelector.ANY, false); } /** diff --git a/client/rest/src/test/java/org/elasticsearch/client/RestClientTests.java b/client/rest/src/test/java/org/elasticsearch/client/RestClientTests.java index 4a037b18404a1..69cdfeae85dff 100644 --- a/client/rest/src/test/java/org/elasticsearch/client/RestClientTests.java +++ b/client/rest/src/test/java/org/elasticsearch/client/RestClientTests.java @@ -57,7 +57,7 @@ public class RestClientTests extends RestClientTestCase { public void testCloseIsIdempotent() throws IOException { List nodes = singletonList(new Node(new HttpHost("localhost", 9200))); CloseableHttpAsyncClient closeableHttpAsyncClient = mock(CloseableHttpAsyncClient.class); - RestClient restClient = new RestClient(closeableHttpAsyncClient, 1_000, new Header[0], nodes, null, null, null); + RestClient restClient = new RestClient(closeableHttpAsyncClient, 1_000, new Header[0], nodes, null, null, null, false); restClient.close(); verify(closeableHttpAsyncClient, times(1)).close(); restClient.close(); @@ -345,7 +345,7 @@ private static String assertSelectAllRejected( NodeTuple> nodeTuple, private static RestClient createRestClient() { List nodes = Collections.singletonList(new Node(new HttpHost("localhost", 9200))); return new RestClient(mock(CloseableHttpAsyncClient.class), randomLongBetween(1_000, 30_000), - new Header[] {}, nodes, null, null, null); + new Header[] {}, nodes, null, null, null, false); } public void testRoundRobin() throws IOException { 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 9d47c4e24a90b..f34513e28c711 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 @@ -82,6 +82,7 @@ public abstract class ESRestTestCase extends ESTestCase { public static final String CLIENT_RETRY_TIMEOUT = "client.retry.timeout"; public static final String CLIENT_SOCKET_TIMEOUT = "client.socket.timeout"; public static final String CLIENT_PATH_PREFIX = "client.path.prefix"; + public static final String STRICT_DEPRECATION_MODE = "strict.deprecation.mode"; /** * Convert the entity from a {@link Response} into a map of maps. @@ -498,6 +499,9 @@ protected static void configureClient(RestClientBuilder builder, Settings settin if (settings.hasValue(CLIENT_PATH_PREFIX)) { builder.setPathPrefix(settings.get(CLIENT_PATH_PREFIX)); } + if (settings.hasValue(STRICT_DEPRECATION_MODE)) { + builder.setStrictDeprecationMode(Boolean.parseBoolean(settings.get(STRICT_DEPRECATION_MODE))); + } } @SuppressWarnings("unchecked") From 38202c92777db7f9f0857c3a1b63c8e28a3b8180 Mon Sep 17 00:00:00 2001 From: lipsill Date: Wed, 26 Sep 2018 13:14:50 +0200 Subject: [PATCH 3/3] addressing reviewers remarks --- .../org/elasticsearch/client/Response.java | 32 +++++++++++-------- .../test/rest/ESRestTestCase.java | 4 --- 2 files changed, 19 insertions(+), 17 deletions(-) diff --git a/client/rest/src/main/java/org/elasticsearch/client/Response.java b/client/rest/src/main/java/org/elasticsearch/client/Response.java index 71309f9f0e88b..ab61f01f66159 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/Response.java +++ b/client/rest/src/main/java/org/elasticsearch/client/Response.java @@ -100,21 +100,23 @@ public HttpEntity getEntity() { return response.getEntity(); } - private static Pattern WARNING_HEADER_PATTERN = Pattern.compile( + private static final Pattern WARNING_HEADER_PATTERN = Pattern.compile( "299 " + // warn code - "Elasticsearch-\\d+\\.\\d+\\.\\d+(?:-(?:alpha|beta|rc)\\d+)?(?:-SNAPSHOT)?-(?:[a-f0-9]{7}|Unknown) " + // warn agent - "\"((?:\t| |!|[\\x23-\\x5B]|[\\x5D-\\x7E]|[\\x80-\\xFF]|\\\\|\\\\\")*)\" " + // quoted warning value, captured - // quoted RFC 1123 date format - "\"" + // opening quote - "(?:Mon|Tue|Wed|Thu|Fri|Sat|Sun), " + // weekday - "\\d{2} " + // 2-digit day - "(?:Jan|Feb|Mar|Apr|May|Jun|Jul|Aug|Sep|Oct|Nov|Dec) " + // month - "\\d{4} " + // 4-digit year - "\\d{2}:\\d{2}:\\d{2} " + // (two-digit hour):(two-digit minute):(two-digit second) - "GMT" + // GMT - "\""); // closing quote - + "Elasticsearch-\\d+\\.\\d+\\.\\d+(?:-(?:alpha|beta|rc)\\d+)?(?:-SNAPSHOT)?-(?:[a-f0-9]{7}|Unknown) " + // warn agent + "\"((?:\t| |!|[\\x23-\\x5B]|[\\x5D-\\x7E]|[\\x80-\\xFF]|\\\\|\\\\\")*)\" " + // quoted warning value, captured + // quoted RFC 1123 date format + "\"" + // opening quote + "(?:Mon|Tue|Wed|Thu|Fri|Sat|Sun), " + // weekday + "\\d{2} " + // 2-digit day + "(?:Jan|Feb|Mar|Apr|May|Jun|Jul|Aug|Sep|Oct|Nov|Dec) " + // month + "\\d{4} " + // 4-digit year + "\\d{2}:\\d{2}:\\d{2} " + // (two-digit hour):(two-digit minute):(two-digit second) + "GMT" + // GMT + "\""); // closing quote + /** + * Returns a list of all warning headers returned in the response. + */ public List getWarnings() { List warnings = new ArrayList<>(); for (Header header : response.getHeaders("Warning")) { @@ -129,6 +131,10 @@ public List getWarnings() { return warnings; } + /** + * Returns true if there is at least one warning header returned in the + * response. + */ public boolean hasWarnings() { Header[] warnings = response.getHeaders("Warning"); return warnings != null && warnings.length > 0; 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 f34513e28c711..9d47c4e24a90b 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 @@ -82,7 +82,6 @@ public abstract class ESRestTestCase extends ESTestCase { public static final String CLIENT_RETRY_TIMEOUT = "client.retry.timeout"; public static final String CLIENT_SOCKET_TIMEOUT = "client.socket.timeout"; public static final String CLIENT_PATH_PREFIX = "client.path.prefix"; - public static final String STRICT_DEPRECATION_MODE = "strict.deprecation.mode"; /** * Convert the entity from a {@link Response} into a map of maps. @@ -499,9 +498,6 @@ protected static void configureClient(RestClientBuilder builder, Settings settin if (settings.hasValue(CLIENT_PATH_PREFIX)) { builder.setPathPrefix(settings.get(CLIENT_PATH_PREFIX)); } - if (settings.hasValue(STRICT_DEPRECATION_MODE)) { - builder.setStrictDeprecationMode(Boolean.parseBoolean(settings.get(STRICT_DEPRECATION_MODE))); - } } @SuppressWarnings("unchecked")