From 335c020cd77a18da46e59e73e08eea8a53303dd3 Mon Sep 17 00:00:00 2001 From: Chris Earle Date: Sat, 27 Aug 2016 01:12:55 -0400 Subject: [PATCH 1/3] Add support for a RestClient base path This enables simple support for proxies (beyond proxy host and proxy port, which is done via the RequestConfig)) to provide a base path in front of all requests performed by the RestClient. --- .../org/elasticsearch/client/RestClient.java | 21 ++++-- .../client/RestClientBuilder.java | 56 +++++++++++++++- .../client/RestClientBuilderTests.java | 59 +++++++++++++---- .../client/RestClientIntegTests.java | 64 +++++++++++++++++++ .../client/RestClientMultipleHostsTests.java | 2 +- .../client/RestClientSingleHostTests.java | 2 +- 6 files changed, 184 insertions(+), 20 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 a6c3d82dd8f..26af479f668 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/RestClient.java +++ b/client/rest/src/main/java/org/elasticsearch/client/RestClient.java @@ -89,17 +89,19 @@ public class RestClient implements Closeable { //we don't rely on default headers supported by HttpAsyncClient as those cannot be replaced private final Header[] defaultHeaders; private final long maxRetryTimeoutMillis; + private final String pathPrefix; private final AtomicInteger lastHostIndex = new AtomicInteger(0); private volatile Set hosts; private final ConcurrentMap blacklist = new ConcurrentHashMap<>(); private final FailureListener failureListener; RestClient(CloseableHttpAsyncClient client, long maxRetryTimeoutMillis, Header[] defaultHeaders, - HttpHost[] hosts, FailureListener failureListener) { + HttpHost[] hosts, String pathPrefix, FailureListener failureListener) { this.client = client; this.maxRetryTimeoutMillis = maxRetryTimeoutMillis; this.defaultHeaders = defaultHeaders; this.failureListener = failureListener; + this.pathPrefix = pathPrefix; setHosts(hosts); } @@ -280,7 +282,7 @@ public class RestClient implements Closeable { public void performRequestAsync(String method, String endpoint, Map params, HttpEntity entity, HttpAsyncResponseConsumer responseConsumer, ResponseListener responseListener, Header... headers) { - URI uri = buildUri(endpoint, params); + URI uri = buildUri(pathPrefix, endpoint, params); HttpRequestBase request = createHttpRequest(method, uri, entity); setHeaders(request, headers); FailureTrackingResponseListener failureTrackingResponseListener = new FailureTrackingResponseListener(responseListener); @@ -501,10 +503,21 @@ public class RestClient implements Closeable { return httpRequest; } - private static URI buildUri(String path, Map params) { + private static URI buildUri(String pathPrefix, String path, Map params) { Objects.requireNonNull(params, "params must not be null"); try { - URIBuilder uriBuilder = new URIBuilder(path); + String fullPath; + if (pathPrefix != null) { + if (path.startsWith("/")) { + fullPath = pathPrefix + path; + } else { + fullPath = pathPrefix + "/" + path; + } + } else { + fullPath = path; + } + + URIBuilder uriBuilder = new URIBuilder(fullPath); for (Map.Entry param : params.entrySet()) { uriBuilder.addParameter(param.getKey(), param.getValue()); } 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 4d5b72eba49..4f9f379d08e 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/RestClientBuilder.java +++ b/client/rest/src/main/java/org/elasticsearch/client/RestClientBuilder.java @@ -51,12 +51,17 @@ public final class RestClientBuilder { private RestClient.FailureListener failureListener; private HttpClientConfigCallback httpClientConfigCallback; private RequestConfigCallback requestConfigCallback; + private String pathPrefix; /** * Creates a new builder instance and sets the hosts that the client will send requests to. + * + * @throws NullPointerException if {@code hosts} or any host is {@code null}. + * @throws IllegalArgumentException if {@code hosts} is empty. */ RestClientBuilder(HttpHost... hosts) { - if (hosts == null || hosts.length == 0) { + Objects.requireNonNull(hosts, "hosts must not be null"); + if (hosts.length == 0) { throw new IllegalArgumentException("no hosts provided"); } for (HttpHost host : hosts) { @@ -67,6 +72,8 @@ public final class RestClientBuilder { /** * Sets the default request headers, which will be sent along with each request + * + * @throws NullPointerException if {@code defaultHeaders} or any header is {@code null}. */ public RestClientBuilder setDefaultHeaders(Header[] defaultHeaders) { Objects.requireNonNull(defaultHeaders, "defaultHeaders must not be null"); @@ -79,6 +86,8 @@ public final class RestClientBuilder { /** * Sets the {@link RestClient.FailureListener} to be notified for each request failure + * + * @throws NullPointerException if {@code failureListener} is {@code null}. */ public RestClientBuilder setFailureListener(RestClient.FailureListener failureListener) { Objects.requireNonNull(failureListener, "failureListener must not be null"); @@ -90,7 +99,7 @@ public final class RestClientBuilder { * Sets the maximum timeout (in milliseconds) to honour in case of multiple retries of the same request. * {@link #DEFAULT_MAX_RETRY_TIMEOUT_MILLIS} if not specified. * - * @throws IllegalArgumentException if maxRetryTimeoutMillis is not greater than 0 + * @throws IllegalArgumentException if {@code maxRetryTimeoutMillis} is not greater than 0 */ public RestClientBuilder setMaxRetryTimeoutMillis(int maxRetryTimeoutMillis) { if (maxRetryTimeoutMillis <= 0) { @@ -102,6 +111,8 @@ public final class RestClientBuilder { /** * Sets the {@link HttpClientConfigCallback} to be used to customize http client configuration + * + * @throws NullPointerException if {@code httpClientConfigCallback} is {@code null}. */ public RestClientBuilder setHttpClientConfigCallback(HttpClientConfigCallback httpClientConfigCallback) { Objects.requireNonNull(httpClientConfigCallback, "httpClientConfigCallback must not be null"); @@ -111,6 +122,8 @@ public final class RestClientBuilder { /** * Sets the {@link RequestConfigCallback} to be used to customize http client configuration + * + * @throws NullPointerException if {@code requestConfigCallback} is {@code null}. */ public RestClientBuilder setRequestConfigCallback(RequestConfigCallback requestConfigCallback) { Objects.requireNonNull(requestConfigCallback, "requestConfigCallback must not be null"); @@ -118,6 +131,43 @@ public final class RestClientBuilder { return this; } + /** + * Sets the path's prefix for every request used by the http client. + *

+ * For example, if this is set to "/my/path", then any client request will become "/my/path/" + endpoint. + *

+ * In essence, every request's {@code endpoint} is prefixed by this {@code pathPrefix}. The path prefix is useful for when + * Elasticsearch is behind a proxy that provides a base path; it is not intended for other purposes and it should not be supplied in + * other scenarios. + * + * @throws NullPointerException if {@code pathPrefix} is {@code null}. + * @throws IllegalArgumentException if {@code pathPrefix} is empty, only '/', or ends with more than one '/'. + */ + public RestClientBuilder setPathPrefix(String pathPrefix) { + Objects.requireNonNull(pathPrefix, "pathPrefix must not be null"); + String cleanPathPrefix = pathPrefix; + + if (cleanPathPrefix.startsWith("/") == false) { + cleanPathPrefix = "/" + cleanPathPrefix; + } + + // best effort to ensure that it looks like "/base/path" rather than "/base/path/" + if (cleanPathPrefix.endsWith("/")) { + cleanPathPrefix = cleanPathPrefix.substring(0, cleanPathPrefix.length() - 1); + + if (cleanPathPrefix.endsWith("/")) { + throw new IllegalArgumentException("pathPrefix is malformed. too many trailing slashes: [" + pathPrefix + "]"); + } + } + + if (cleanPathPrefix.isEmpty() || "/".equals(cleanPathPrefix)) { + throw new IllegalArgumentException("pathPrefix must not be empty or '/': [" + pathPrefix + "]"); + } + + this.pathPrefix = cleanPathPrefix; + return this; + } + /** * Creates a new {@link RestClient} based on the provided configuration. */ @@ -126,7 +176,7 @@ public final class RestClientBuilder { failureListener = new RestClient.FailureListener(); } CloseableHttpAsyncClient httpClient = createHttpClient(); - RestClient restClient = new RestClient(httpClient, maxRetryTimeout, defaultHeaders, hosts, failureListener); + RestClient restClient = new RestClient(httpClient, maxRetryTimeout, defaultHeaders, hosts, pathPrefix, failureListener); httpClient.start(); return restClient; } diff --git a/client/rest/src/test/java/org/elasticsearch/client/RestClientBuilderTests.java b/client/rest/src/test/java/org/elasticsearch/client/RestClientBuilderTests.java index ca671862124..c9243d3aaf6 100644 --- a/client/rest/src/test/java/org/elasticsearch/client/RestClientBuilderTests.java +++ b/client/rest/src/test/java/org/elasticsearch/client/RestClientBuilderTests.java @@ -19,7 +19,6 @@ package org.elasticsearch.client; -import com.carrotsearch.randomizedtesting.generators.RandomInts; import org.apache.http.Header; import org.apache.http.HttpHost; import org.apache.http.client.config.RequestConfig; @@ -28,8 +27,10 @@ import org.apache.http.message.BasicHeader; import java.io.IOException; +import static org.hamcrest.Matchers.containsString; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertThat; import static org.junit.Assert.fail; public class RestClientBuilderTests extends RestClientTestCase { @@ -38,8 +39,8 @@ public class RestClientBuilderTests extends RestClientTestCase { try { RestClient.builder((HttpHost[])null); fail("should have failed"); - } catch(IllegalArgumentException e) { - assertEquals("no hosts provided", e.getMessage()); + } catch(NullPointerException e) { + assertEquals("hosts must not be null", e.getMessage()); } try { @@ -62,7 +63,7 @@ public class RestClientBuilderTests extends RestClientTestCase { try { RestClient.builder(new HttpHost("localhost", 9200)) - .setMaxRetryTimeoutMillis(RandomInts.randomIntBetween(getRandom(), Integer.MIN_VALUE, 0)); + .setMaxRetryTimeoutMillis(randomIntBetween(Integer.MIN_VALUE, 0)); fail("should have failed"); } catch(IllegalArgumentException e) { assertEquals("maxRetryTimeoutMillis must be greater than 0", e.getMessage()); @@ -103,13 +104,13 @@ public class RestClientBuilderTests extends RestClientTestCase { assertEquals("requestConfigCallback must not be null", e.getMessage()); } - int numNodes = RandomInts.randomIntBetween(getRandom(), 1, 5); + int numNodes = randomIntBetween(1, 5); HttpHost[] hosts = new HttpHost[numNodes]; for (int i = 0; i < numNodes; i++) { hosts[i] = new HttpHost("localhost", 9200 + i); } RestClientBuilder builder = RestClient.builder(hosts); - if (getRandom().nextBoolean()) { + if (randomBoolean()) { builder.setHttpClientConfigCallback(new RestClientBuilder.HttpClientConfigCallback() { @Override public HttpAsyncClientBuilder customizeHttpClient(HttpAsyncClientBuilder httpClientBuilder) { @@ -117,7 +118,7 @@ public class RestClientBuilderTests extends RestClientTestCase { } }); } - if (getRandom().nextBoolean()) { + if (randomBoolean()) { builder.setRequestConfigCallback(new RestClientBuilder.RequestConfigCallback() { @Override public RequestConfig.Builder customizeRequestConfig(RequestConfig.Builder requestConfigBuilder) { @@ -125,19 +126,55 @@ public class RestClientBuilderTests extends RestClientTestCase { } }); } - if (getRandom().nextBoolean()) { - int numHeaders = RandomInts.randomIntBetween(getRandom(), 1, 5); + if (randomBoolean()) { + int numHeaders = randomIntBetween(1, 5); Header[] headers = new Header[numHeaders]; for (int i = 0; i < numHeaders; i++) { headers[i] = new BasicHeader("header" + i, "value"); } builder.setDefaultHeaders(headers); } - if (getRandom().nextBoolean()) { - builder.setMaxRetryTimeoutMillis(RandomInts.randomIntBetween(getRandom(), 1, Integer.MAX_VALUE)); + if (randomBoolean()) { + builder.setMaxRetryTimeoutMillis(randomIntBetween(1, Integer.MAX_VALUE)); + } + if (randomBoolean()) { + String pathPrefix = (randomBoolean() ? "/" : "") + randomAsciiOfLengthBetween(2, 5); + while (pathPrefix.length() < 20 && randomBoolean()) { + pathPrefix += "/" + randomAsciiOfLengthBetween(3, 6); + } + builder.setPathPrefix(pathPrefix + (randomBoolean() ? "/" : "")); } try (RestClient restClient = builder.build()) { assertNotNull(restClient); } } + + public void testSetPathPrefixNull() { + try { + RestClient.builder(new HttpHost("localhost", 9200)).setPathPrefix(null); + fail("pathPrefix set to null should fail!"); + } catch (final NullPointerException e) { + assertEquals("pathPrefix must not be null", e.getMessage()); + } + } + + public void testSetPathPrefixEmpty() { + assertSetPathPrefixThrows("/"); + assertSetPathPrefixThrows(""); + } + + public void testSetPathPrefixMalformed() { + assertSetPathPrefixThrows("//"); + assertSetPathPrefixThrows("base/path//"); + } + + private static void assertSetPathPrefixThrows(final String pathPrefix) { + try { + RestClient.builder(new HttpHost("localhost", 9200)).setPathPrefix(pathPrefix); + fail("path prefix [" + pathPrefix + "] should have failed"); + } catch (final IllegalArgumentException e) { + assertThat(e.getMessage(), containsString(pathPrefix)); + } + } + } diff --git a/client/rest/src/test/java/org/elasticsearch/client/RestClientIntegTests.java b/client/rest/src/test/java/org/elasticsearch/client/RestClientIntegTests.java index 79da041f379..a6f6829c60f 100644 --- a/client/rest/src/test/java/org/elasticsearch/client/RestClientIntegTests.java +++ b/client/rest/src/test/java/org/elasticsearch/client/RestClientIntegTests.java @@ -22,6 +22,7 @@ package org.elasticsearch.client; import com.carrotsearch.randomizedtesting.generators.RandomInts; import com.carrotsearch.randomizedtesting.generators.RandomStrings; import com.sun.net.httpserver.Headers; +import com.sun.net.httpserver.HttpContext; import com.sun.net.httpserver.HttpExchange; import com.sun.net.httpserver.HttpHandler; import com.sun.net.httpserver.HttpServer; @@ -60,6 +61,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; /** * Integration test to check interaction between {@link RestClient} and {@link org.apache.http.client.HttpClient}. @@ -205,6 +207,68 @@ public class RestClientIntegTests extends RestClientTestCase { bodyTest("GET"); } + /** + * Ensure that pathPrefix works as expected even when the path does not exist. + */ + public void testPathPrefixUnknownPath() throws IOException { + // guarantee no other test setup collides with this one and lets it sneak through + final String uniqueContextSuffix = "/testPathPrefixUnknownPath"; + final String pathPrefix = "dne/" + randomAsciiOfLengthBetween(1, 5) + "/"; + final int statusCode = randomStatusCode(getRandom()); + + try (final RestClient client = + RestClient.builder(new HttpHost(httpServer.getAddress().getHostString(), httpServer.getAddress().getPort())) + .setPathPrefix((randomBoolean() ? "/" : "") + pathPrefix).build()) { + + for (final String method : getHttpMethods()) { + Response esResponse; + try { + esResponse = client.performRequest(method, "/" + statusCode + uniqueContextSuffix); + if ("HEAD".equals(method) == false) { + fail("only HEAD requests should not throw an exception; 404 is expected"); + } + } catch(ResponseException e) { + esResponse = e.getResponse(); + } + + assertThat(esResponse.getRequestLine().getUri(), equalTo("/" + pathPrefix + statusCode + uniqueContextSuffix)); + assertThat(esResponse.getStatusLine().getStatusCode(), equalTo(404)); + } + } + } + + /** + * Ensure that pathPrefix works as expected. + */ + public void testPathPrefix() throws IOException { + // guarantee no other test setup collides with this one and lets it sneak through + final String uniqueContextSuffix = "/testPathPrefix"; + final String pathPrefix = "base/" + randomAsciiOfLengthBetween(1, 5) + "/"; + final int statusCode = randomStatusCode(getRandom()); + + final HttpContext context = + httpServer.createContext("/" + pathPrefix + statusCode + uniqueContextSuffix, new ResponseHandler(statusCode)); + + try (final RestClient client = + RestClient.builder(new HttpHost(httpServer.getAddress().getHostString(), httpServer.getAddress().getPort())) + .setPathPrefix((randomBoolean() ? "/" : "") + pathPrefix).build()) { + + for (final String method : getHttpMethods()) { + Response esResponse; + try { + esResponse = client.performRequest(method, "/" + statusCode + uniqueContextSuffix); + } catch(ResponseException e) { + esResponse = e.getResponse(); + } + + assertThat(esResponse.getRequestLine().getUri(), equalTo("/" + pathPrefix + statusCode + uniqueContextSuffix)); + assertThat(esResponse.getStatusLine().getStatusCode(), equalTo(statusCode)); + } + } finally { + httpServer.removeContext(context); + } + } + private void bodyTest(String method) throws IOException { String requestBody = "{ \"field\": \"value\" }"; StringEntity entity = new StringEntity(requestBody); 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 2a2d279689a..049a216936f 100644 --- a/client/rest/src/test/java/org/elasticsearch/client/RestClientMultipleHostsTests.java +++ b/client/rest/src/test/java/org/elasticsearch/client/RestClientMultipleHostsTests.java @@ -101,7 +101,7 @@ public class RestClientMultipleHostsTests extends RestClientTestCase { httpHosts[i] = new HttpHost("localhost", 9200 + i); } failureListener = new HostsTrackingFailureListener(); - restClient = new RestClient(httpClient, 10000, new Header[0], httpHosts, failureListener); + restClient = new RestClient(httpClient, 10000, new Header[0], httpHosts, null, failureListener); } public void testRoundRobinOkStatusCodes() throws IOException { 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 e347dfecc12..a6ae30b01e8 100644 --- a/client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostTests.java +++ b/client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostTests.java @@ -141,7 +141,7 @@ public class RestClientSingleHostTests extends RestClientTestCase { } httpHost = new HttpHost("localhost", 9200); failureListener = new HostsTrackingFailureListener(); - restClient = new RestClient(httpClient, 10000, defaultHeaders, new HttpHost[]{httpHost}, failureListener); + restClient = new RestClient(httpClient, 10000, defaultHeaders, new HttpHost[]{httpHost}, null, failureListener); } /** From e19f2b63482f6a8586218e802e953002df41b357 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Tue, 30 Aug 2016 13:55:44 -0700 Subject: [PATCH 2/3] Tests: Improve rest suite names and generated test names for docs tests Rest test suites are currently only the directory above the yaml test file. That is confusing when there are more than one directory level which contain yaml tests, as there are in generated docs tests. This change makes rest tests use the full relative path to the rest test root as the suite name, and also makes the test names for docs tests a little clearer (that they are testing an example from a specific line number, instead of just the line number as an opaque test name). --- .../elasticsearch/gradle/doc/RestTestsFromSnippetsTask.groovy | 2 +- .../main/java/org/elasticsearch/test/rest/yaml/FileUtils.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/buildSrc/src/main/groovy/org/elasticsearch/gradle/doc/RestTestsFromSnippetsTask.groovy b/buildSrc/src/main/groovy/org/elasticsearch/gradle/doc/RestTestsFromSnippetsTask.groovy index 61a07f4fbd4..fc7604ad1fd 100644 --- a/buildSrc/src/main/groovy/org/elasticsearch/gradle/doc/RestTestsFromSnippetsTask.groovy +++ b/buildSrc/src/main/groovy/org/elasticsearch/gradle/doc/RestTestsFromSnippetsTask.groovy @@ -117,7 +117,7 @@ public class RestTestsFromSnippetsTask extends SnippetsTask { if (false == test.continued) { current.println('---') - current.println("\"$test.start\":") + current.println("\"line_$test.start\":") } if (test.skipTest) { current.println(" - skip:") diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/FileUtils.java b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/FileUtils.java index caaa8b2ec83..4519953819a 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/FileUtils.java +++ b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/FileUtils.java @@ -155,7 +155,7 @@ public final class FileUtils { @Override public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException { if (file.toString().endsWith(fileSuffix)) { - String groupName = file.toAbsolutePath().getParent().getFileName().toString(); + String groupName = dir.relativize(file.getParent()).toString(); Set filesSet = files.get(groupName); if (filesSet == null) { filesSet = new HashSet<>(); From c05d5f925789154e62cb0bd94611cd4aedc49755 Mon Sep 17 00:00:00 2001 From: Chris Earle Date: Tue, 30 Aug 2016 17:18:01 -0400 Subject: [PATCH 3/3] Remove unknown HttpContext-based test as it fails unpredictably on different JVMs --- .../client/RestClientIntegTests.java | 30 ------------------- 1 file changed, 30 deletions(-) diff --git a/client/rest/src/test/java/org/elasticsearch/client/RestClientIntegTests.java b/client/rest/src/test/java/org/elasticsearch/client/RestClientIntegTests.java index a6f6829c60f..e7d7852de04 100644 --- a/client/rest/src/test/java/org/elasticsearch/client/RestClientIntegTests.java +++ b/client/rest/src/test/java/org/elasticsearch/client/RestClientIntegTests.java @@ -207,36 +207,6 @@ public class RestClientIntegTests extends RestClientTestCase { bodyTest("GET"); } - /** - * Ensure that pathPrefix works as expected even when the path does not exist. - */ - public void testPathPrefixUnknownPath() throws IOException { - // guarantee no other test setup collides with this one and lets it sneak through - final String uniqueContextSuffix = "/testPathPrefixUnknownPath"; - final String pathPrefix = "dne/" + randomAsciiOfLengthBetween(1, 5) + "/"; - final int statusCode = randomStatusCode(getRandom()); - - try (final RestClient client = - RestClient.builder(new HttpHost(httpServer.getAddress().getHostString(), httpServer.getAddress().getPort())) - .setPathPrefix((randomBoolean() ? "/" : "") + pathPrefix).build()) { - - for (final String method : getHttpMethods()) { - Response esResponse; - try { - esResponse = client.performRequest(method, "/" + statusCode + uniqueContextSuffix); - if ("HEAD".equals(method) == false) { - fail("only HEAD requests should not throw an exception; 404 is expected"); - } - } catch(ResponseException e) { - esResponse = e.getResponse(); - } - - assertThat(esResponse.getRequestLine().getUri(), equalTo("/" + pathPrefix + statusCode + uniqueContextSuffix)); - assertThat(esResponse.getStatusLine().getStatusCode(), equalTo(404)); - } - } - } - /** * Ensure that pathPrefix works as expected. */