Fix RestClientOptions building to not lose builtin headers (#380) (#382)

Co-authored-by: Sylvain Wallez <[email protected]>

Author: github-actions[bot]

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<T extends Transport, Self extends ApiClient<T, Self>> {
 
@@ -52,15 +53,27 @@ protected <V> JsonpDeserializer<V> getDeserializer(Class<V> 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<TransportOptions.Builder, TransportOptions.Builder> fn) {
+        return withTransportOptions(fn.apply(_transportOptions().toBuilder()).build());
+    }
+
     /**
      * Get the transport used by this client to handle communication with the server.
      */
     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;
     }
 
     /**

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<List<String>, 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(

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.
+ *
+ * <p><b>Do not use this interface</b> 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.
+ *
+ * <p>Borrowed from <a href="https://github.com/google/guava/blob/master/guava/src/com/google/common/annotations/VisibleForTesting.java">
+ *    Guava</a>.
+ */
+@Target({ElementType.METHOD, ElementType.FIELD, ElementType.CONSTRUCTOR, ElementType.TYPE})
+@Retention(RetentionPolicy.SOURCE)
+public @interface VisibleForTesting {
+}

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);