From 7308c5e734f53c854d9a1cb8246615505e6144fb Mon Sep 17 00:00:00 2001 From: Sylvain Wallez Date: Thu, 18 Aug 2022 18:21:21 +0200 Subject: [PATCH] Fix RestClientOptions building to not loose builtin headers --- .../java/co/elastic/clients/ApiClient.java | 15 +- .../rest_client/RestClientOptions.java | 64 +++--- .../clients/util/VisibleForTesting.java | 41 ++++ .../clients/transport/RequestOptionsTest.java | 18 +- .../rest_client/RestClientOptionsTest.java | 189 ++++++++++++++++++ 5 files changed, 291 insertions(+), 36 deletions(-) create mode 100644 java-client/src/main/java/co/elastic/clients/util/VisibleForTesting.java create mode 100644 java-client/src/test/java/co/elastic/clients/transport/rest_client/RestClientOptionsTest.java diff --git a/java-client/src/main/java/co/elastic/clients/ApiClient.java b/java-client/src/main/java/co/elastic/clients/ApiClient.java index 7e5d6b342..29f03c3f9 100644 --- a/java-client/src/main/java/co/elastic/clients/ApiClient.java +++ b/java-client/src/main/java/co/elastic/clients/ApiClient.java @@ -26,6 +26,7 @@ import co.elastic.clients.json.JsonpMapperBase; import javax.annotation.Nullable; +import java.util.function.Function; public abstract class ApiClient> { @@ -52,6 +53,15 @@ protected JsonpDeserializer getDeserializer(Class clazz) { */ public abstract Self withTransportOptions(@Nullable TransportOptions transportOptions); + /** + * Creates a new client with additional request options + * + * @param fn a lambda expression that takes the current options as input + */ + public Self withTransportOptions(Function fn) { + return withTransportOptions(fn.apply(_transportOptions().toBuilder()).build()); + } + /** * Get the transport used by this client to handle communication with the server. */ @@ -59,8 +69,11 @@ public T _transport() { return this.transport; } + /** + * Get the transport options used for this client. If the client has no custom options, falls back to the transport's options. + */ public TransportOptions _transportOptions() { - return this.transportOptions; + return this.transportOptions == null ? transport.options() : transportOptions; } /** diff --git a/java-client/src/main/java/co/elastic/clients/transport/rest_client/RestClientOptions.java b/java-client/src/main/java/co/elastic/clients/transport/rest_client/RestClientOptions.java index 5d7e9a098..5c4edde77 100644 --- a/java-client/src/main/java/co/elastic/clients/transport/rest_client/RestClientOptions.java +++ b/java-client/src/main/java/co/elastic/clients/transport/rest_client/RestClientOptions.java @@ -21,6 +21,7 @@ import co.elastic.clients.transport.TransportOptions; import co.elastic.clients.transport.Version; +import co.elastic.clients.util.VisibleForTesting; import org.apache.http.impl.nio.client.HttpAsyncClientBuilder; import org.apache.http.util.VersionInfo; import org.elasticsearch.client.RequestOptions; @@ -38,6 +39,14 @@ public class RestClientOptions implements TransportOptions { private final RequestOptions options; + private static final String CLIENT_META_HEADER = "X-Elastic-Client-Meta"; + private static final String USER_AGENT_HEADER = "User-Agent"; + + @VisibleForTesting + static final String CLIENT_META_VALUE = getClientMeta(); + @VisibleForTesting + static final String USER_AGENT_VALUE = getUserAgent(); + static RestClientOptions of(TransportOptions options) { if (options instanceof RestClientOptions) { return (RestClientOptions)options; @@ -52,7 +61,7 @@ static RestClientOptions of(TransportOptions options) { } public RestClientOptions(RequestOptions options) { - this.options = options; + this.options = addBuiltinHeaders(options.toBuilder()).build(); } /** @@ -109,23 +118,13 @@ public RequestOptions.Builder restClientRequestOptionsBuilder() { @Override public TransportOptions.Builder addHeader(String name, String value) { - if (name.equalsIgnoreCase(CLIENT_META)) { + if (name.equalsIgnoreCase(CLIENT_META_HEADER)) { + // Not overridable return this; } - if (name.equalsIgnoreCase(USER_AGENT)) { - // We must filter out our own user-agent from the options or they'll end up as multiple values for the header - RequestOptions options = builder.build(); - builder = RequestOptions.DEFAULT.toBuilder(); - options.getParameters().forEach((k, v) -> builder.addParameter(k, v)); - options.getHeaders().forEach(h -> { - if (!h.getName().equalsIgnoreCase(USER_AGENT)) { - builder.addHeader(h.getName(), h.getValue()); - } - }); - builder.setWarningsHandler(options.getWarningsHandler()); - if (options.getHttpAsyncResponseConsumerFactory() != null) { - builder.setHttpAsyncResponseConsumerFactory(options.getHttpAsyncResponseConsumerFactory()); - } + if (name.equalsIgnoreCase(USER_AGENT_HEADER)) { + // We must remove our own user-agent from the options, or we'll end up with multiple values for the header + builder.removeHeader(USER_AGENT_HEADER); } builder.addHeader(name, value); return this; @@ -159,32 +158,37 @@ public TransportOptions.Builder onWarnings(Function, Boolean> liste @Override public RestClientOptions build() { - return new RestClientOptions(builder.build()); + return new RestClientOptions(addBuiltinHeaders(builder).build()); } } - private static final String USER_AGENT = "User-Agent"; - private static final String CLIENT_META = "X-Elastic-Client-Meta"; - static RestClientOptions initialOptions() { - String ua = String.format( + return new RestClientOptions(RequestOptions.DEFAULT); + } + + private static RequestOptions.Builder addBuiltinHeaders(RequestOptions.Builder builder) { + builder.removeHeader(CLIENT_META_HEADER); + builder.addHeader(CLIENT_META_HEADER, CLIENT_META_VALUE); + if (builder.getHeaders().stream().noneMatch(h -> h.getName().equalsIgnoreCase(USER_AGENT_HEADER))) { + builder.addHeader(USER_AGENT_HEADER, USER_AGENT_VALUE); + } + if (builder.getHeaders().stream().noneMatch(h -> h.getName().equalsIgnoreCase("Accept"))) { + builder.addHeader("Accept", RestClientTransport.JsonContentType.toString()); + } + + return builder; + } + + private static String getUserAgent() { + return String.format( Locale.ROOT, "elastic-java/%s (Java/%s)", Version.VERSION == null ? "Unknown" : Version.VERSION.toString(), System.getProperty("java.version") ); - - return new RestClientOptions( - RequestOptions.DEFAULT.toBuilder() - .addHeader(USER_AGENT, ua) - .addHeader(CLIENT_META, getClientMeta()) - .addHeader("Accept", RestClientTransport.JsonContentType.toString()) - .build() - ); } private static String getClientMeta() { - VersionInfo httpClientVersion = null; try { httpClientVersion = VersionInfo.loadVersionInfo( diff --git a/java-client/src/main/java/co/elastic/clients/util/VisibleForTesting.java b/java-client/src/main/java/co/elastic/clients/util/VisibleForTesting.java new file mode 100644 index 000000000..d4e1585c9 --- /dev/null +++ b/java-client/src/main/java/co/elastic/clients/util/VisibleForTesting.java @@ -0,0 +1,41 @@ +/* + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package co.elastic.clients.util; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * Annotates a program element that exists, or is more widely visible than otherwise necessary, only + * for use in test code. + * + *

Do not use this interface for public or protected declarations: it is a fig leaf for + * bad design, and it does not prevent anyone from using the declaration---and experience has shown + * that they will. + * + *

Borrowed from + * Guava. + */ +@Target({ElementType.METHOD, ElementType.FIELD, ElementType.CONSTRUCTOR, ElementType.TYPE}) +@Retention(RetentionPolicy.SOURCE) +public @interface VisibleForTesting { +} diff --git a/java-client/src/test/java/co/elastic/clients/transport/RequestOptionsTest.java b/java-client/src/test/java/co/elastic/clients/transport/RequestOptionsTest.java index 43c56f612..c7927f980 100644 --- a/java-client/src/test/java/co/elastic/clients/transport/RequestOptionsTest.java +++ b/java-client/src/test/java/co/elastic/clients/transport/RequestOptionsTest.java @@ -100,6 +100,15 @@ private Properties getProps(ElasticsearchClient client) throws IOException { return result; } + @Test + public void testNonNullClientOptions() { + final RestClientTransport trsp = new RestClientTransport(restClient, new JsonbJsonpMapper()); + final ElasticsearchClient client = new ElasticsearchClient(trsp); + + assertNotNull(client._transportOptions()); + assertSame(trsp.options(), client._transportOptions()); + } + @Test public void testDefaultHeaders() throws IOException { final RestClientTransport trsp = new RestClientTransport(restClient, new JsonbJsonpMapper()); @@ -111,7 +120,7 @@ public void testDefaultHeaders() throws IOException { assertTrue(props.getProperty("header-x-elastic-client-meta").contains("es=")); assertTrue(props.getProperty("header-x-elastic-client-meta").contains("hl=2")); assertEquals( - "application/vnd.elasticsearch+json; compatible-with=" + String.valueOf(Version.VERSION.major()), + "application/vnd.elasticsearch+json; compatible-with=" + Version.VERSION.major(), props.getProperty("header-accept") ); } @@ -120,10 +129,9 @@ public void testDefaultHeaders() throws IOException { public void testClientHeader() throws IOException { final RestClientTransport trsp = new RestClientTransport(restClient, new JsonbJsonpMapper()); final ElasticsearchClient client = new ElasticsearchClient(trsp) - .withTransportOptions(trsp.options().with( - b -> b.addHeader("X-Foo", "Bar") - .addHeader("uSer-agEnt", "MegaClient/1.2.3") - ) + .withTransportOptions(b -> b + .addHeader("X-Foo", "Bar") + .addHeader("uSer-agEnt", "MegaClient/1.2.3") ); Properties props = getProps(client); diff --git a/java-client/src/test/java/co/elastic/clients/transport/rest_client/RestClientOptionsTest.java b/java-client/src/test/java/co/elastic/clients/transport/rest_client/RestClientOptionsTest.java new file mode 100644 index 000000000..488e55ff1 --- /dev/null +++ b/java-client/src/test/java/co/elastic/clients/transport/rest_client/RestClientOptionsTest.java @@ -0,0 +1,189 @@ +/* + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package co.elastic.clients.transport.rest_client; + +import co.elastic.clients.elasticsearch.ElasticsearchClient; +import co.elastic.clients.json.SimpleJsonpMapper; +import co.elastic.clients.transport.Version; +import co.elastic.clients.transport.endpoints.BooleanResponse; +import com.sun.net.httpserver.Headers; +import com.sun.net.httpserver.HttpServer; +import org.apache.http.HttpHost; +import org.elasticsearch.client.RequestOptions; +import org.elasticsearch.client.RestClient; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.net.InetAddress; +import java.net.InetSocketAddress; +import java.util.List; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicInteger; + +class RestClientOptionsTest extends Assertions { + + /** Collected headers by test name */ + private static Map collectedHeaders; + private static final AtomicInteger testCounter = new AtomicInteger(); + private static HttpServer httpServer; + + private static final String MIME_TYPE = "application/vnd.elasticsearch+json; compatible-with=" + Version.VERSION.major(); + + @BeforeAll + public static void setup() throws IOException { + collectedHeaders = new ConcurrentHashMap<>(); + httpServer = HttpServer.create(new InetSocketAddress(InetAddress.getLoopbackAddress(), 0), 0); + + // Register a handler on the core.exists("capture-handler/{name}") endpoint that will capture request headers. + httpServer.createContext("/capture-headers/_doc/", exchange -> { + String testName = exchange.getRequestURI().getPath().substring("/capture-headers/_doc/".length()); + collectedHeaders.put(testName, exchange.getRequestHeaders()); + + // Reply with an empty 200 response + exchange.getResponseHeaders().set("X-Elastic-Product", "Elasticsearch"); + exchange.sendResponseHeaders(200, -1); + exchange.close(); + }); + + httpServer.start(); + } + + @AfterAll + public static void cleanup() { + httpServer.stop(0); + httpServer = null; + collectedHeaders = null; + } + + /** + * Make a server call, capture request headers and check their consistency. + * + * @return the name of the entry in collectedHeaders for further inspection. + */ + private String checkHeaders(ElasticsearchClient esClient) throws IOException { + String testName = "test-" + testCounter.incrementAndGet(); + BooleanResponse exists = esClient.exists(r -> r.index("capture-headers").id(testName)); + assertTrue(exists.value()); + + Headers headers = collectedHeaders.get(testName); + assertNotNull(headers, "No headers collected for test " + testName); + + assertNotNull(headers.get("X-elastic-client-meta"), "Missing client meta header"); + assertEquals(RestClientOptions.CLIENT_META_VALUE, headers.get("X-elastic-client-meta").get(0)); + assertNotNull(headers.get("Accept"), "Missing 'Accept' header"); + assertEquals(MIME_TYPE, headers.get("Accept").get(0)); + + for (Map.Entry> entry: headers.entrySet()) { + System.out.println(entry.getKey() + " " + entry.getValue()); + } + + return testName; + } + + private void checkUserAgent(String testName, String value) { + Headers headers = collectedHeaders.get(testName); + assertNotNull(headers, "No headers collected for test " + testName); + assertNotNull(headers.get("User-Agent"), "Missing 'User-Agent' header"); + assertEquals(value, headers.get("User-Agent").get(0)); + } + + @Test + void testNoRequestOptions() throws Exception { + RestClient llrc = RestClient.builder( + new HttpHost(httpServer.getAddress().getHostString(), httpServer.getAddress().getPort(), "http") + ).build(); + + RestClientTransport transport = new RestClientTransport(llrc, new SimpleJsonpMapper()); + ElasticsearchClient esClient = new ElasticsearchClient(transport); + + String id = checkHeaders(esClient); + checkUserAgent(id, RestClientOptions.USER_AGENT_VALUE); + } + + @Test + void testTransportRequestOptions() throws Exception { + RestClient llrc = RestClient.builder( + new HttpHost(httpServer.getAddress().getHostString(), httpServer.getAddress().getPort(), "http") + ).build(); + + RestClientTransport transport = new RestClientTransport(llrc, new SimpleJsonpMapper(), + new RestClientOptions.Builder(RequestOptions.DEFAULT.toBuilder()).build() + ); + ElasticsearchClient esClient = new ElasticsearchClient(transport); + + String id = checkHeaders(esClient); + checkUserAgent(id, RestClientOptions.USER_AGENT_VALUE); + } + + @Test + void testClientRequestOptions() throws Exception { + RestClient llrc = RestClient.builder( + new HttpHost(httpServer.getAddress().getHostString(), httpServer.getAddress().getPort(), "http") + ).build(); + + RestClientTransport transport = new RestClientTransport(llrc, new SimpleJsonpMapper()); + ElasticsearchClient esClient = new ElasticsearchClient(transport).withTransportOptions( + new RestClientOptions.Builder(RequestOptions.DEFAULT.toBuilder()).build() + ); + + String id = checkHeaders(esClient); + checkUserAgent(id, RestClientOptions.USER_AGENT_VALUE); + } + + @Test + void testLambdaOptionsBuilder() throws Exception { + RestClient llrc = RestClient.builder( + new HttpHost(httpServer.getAddress().getHostString(), httpServer.getAddress().getPort(), "http") + ).build(); + + RestClientTransport transport = new RestClientTransport(llrc, new SimpleJsonpMapper()); + ElasticsearchClient esClient = new ElasticsearchClient(transport) + .withTransportOptions(o -> o + .addHeader("Foo", "bar") + .addHeader("x-elastic-client-meta", "foo-bar-client") + ); + + String id = checkHeaders(esClient); + checkUserAgent(id, RestClientOptions.USER_AGENT_VALUE); + } + + @Test + void testRequestOptionsOverridingBuiltin() throws Exception { + RequestOptions options = RequestOptions.DEFAULT.toBuilder() + .addHeader("user-agent", "FooBarAgent/1.0") + .addHeader("x-elastic-client-meta", "foo-bar-client") + .build(); + + RestClient llrc = RestClient.builder( + new HttpHost(httpServer.getAddress().getHostString(), httpServer.getAddress().getPort(), "http") + ).build(); + + RestClientTransport transport = new RestClientTransport(llrc, new SimpleJsonpMapper(), new RestClientOptions(options)); + ElasticsearchClient esClient = new ElasticsearchClient(transport); + // Should not override client meta + String id = checkHeaders(esClient); + // overriding user-agent is ok + checkUserAgent(id, "FooBarAgent/1.0"); + } +}