Revert "Revert "Jonmv/apache hc5 client""

3 files changed, 57 insertions, 53 deletions

diff --git a/configserver-client/pom.xml b/configserver-client/pom.xml
index cee6f6c067f..0a29ba003f4 100644
--- a/configserver-client/pom.xml
+++ b/configserver-client/pom.xml
@@ -55,13 +55,7 @@
 
         <dependency>
             <groupId>org.junit.jupiter</groupId>
-            <artifactId>junit-jupiter-engine</artifactId>
-            <scope>test</scope>
-        </dependency>
-        <dependency>
-            <groupId>com.yahoo.vespa</groupId>
-            <artifactId>testutil</artifactId>
-            <version>${project.version}</version>
+            <artifactId>junit-jupiter</artifactId>
             <scope>test</scope>
         </dependency>
         <dependency>
diff --git a/configserver-client/src/main/java/ai/vespa/hosted/client/AbstractConfigServerClient.java b/configserver-client/src/main/java/ai/vespa/hosted/client/AbstractConfigServerClient.java
index 0ee9e320259..e11f0db7194 100644
--- a/configserver-client/src/main/java/ai/vespa/hosted/client/AbstractConfigServerClient.java
+++ b/configserver-client/src/main/java/ai/vespa/hosted/client/AbstractConfigServerClient.java
@@ -52,7 +52,7 @@ public abstract class AbstractConfigServerClient implements ConfigServerClient {
     protected abstract ClassicHttpResponse execute(ClassicHttpRequest request, HttpClientContext context) throws IOException;
 
     /** Executes the given request with response/error handling and retries. */
-    private <T> T execute(RequestBuilder builder, BiFunction<ClassicHttpResponse, IOException, T> handler) {
+    private <T> T execute(RequestBuilder builder, Function<ClassicHttpResponse, T> handler, Function<IOException, T> catcher) {
         HttpClientContext context = HttpClientContext.create();
         context.setRequestConfig(builder.config);
 
@@ -62,10 +62,10 @@ public abstract class AbstractConfigServerClient implements ConfigServerClient {
             request.setEntity(builder.entity);
             try {
                 try {
-                    return handler.apply(execute(request, context), null);
+                    return handler.apply(execute(request, context));
                 }
                 catch (IOException e) {
-                    return handler.apply(null, e);
+                    return catcher.apply(e);
                 }
             }
             catch (RetryException e) {
@@ -133,7 +133,7 @@ public abstract class AbstractConfigServerClient implements ConfigServerClient {
         }
 
         @Override
-        public RequestBuilder at(String... pathSegments) {
+        public RequestBuilder at(List<String> pathSegments) {
             uriBuilder.setPathSegments(requireNonNull(pathSegments));
             return this;
         }
@@ -150,19 +150,19 @@ public abstract class AbstractConfigServerClient implements ConfigServerClient {
         }
 
         @Override
-        public RequestBuilder parameters(String... pairs) {
-            if (pairs.length % 2 != 0)
+        public RequestBuilder parameters(List<String> pairs) {
+            if (pairs.size() % 2 != 0)
                 throw new IllegalArgumentException("Must supply parameter key/values in pairs");
 
-            for (int i = 0; i < pairs.length; )
-                uriBuilder.setParameter(pairs[i++], pairs[i++]);
+            for (int i = 0; i < pairs.size(); )
+                uriBuilder.setParameter(pairs.get(i++), pairs.get(i++));
 
             return this;
         }
 
         @Override
         public RequestBuilder timeout(Duration timeout) {
-            return config(RequestConfig.copy(defaultRequestConfig)
+            return config(RequestConfig.copy(config)
                                        .setResponseTimeout(timeout.toMillis(), TimeUnit.MILLISECONDS)
                                        .build());
         }
@@ -175,8 +175,8 @@ public abstract class AbstractConfigServerClient implements ConfigServerClient {
         }
 
         @Override
-        public <T> T handle(BiFunction<ClassicHttpResponse, IOException, T> handler) throws UncheckedIOException {
-            return execute(this, requireNonNull(handler));
+        public <T> T handle(Function<ClassicHttpResponse, T> handler, Function<IOException, T> catcher) throws UncheckedIOException {
+            return execute(this, requireNonNull(handler), requireNonNull(catcher));
         }
 
         @Override
@@ -210,37 +210,38 @@ public abstract class AbstractConfigServerClient implements ConfigServerClient {
 
         /** Returns the mapped body, if successful, retrying any IOException. The caller must close the body stream. */
         private <T> T mapIfSuccess(Function<InputStream, T> mapper) {
-            return handle((response, ioException) -> {
-                if (response != null) {
-                    try {
-                        InputStream body = response.getEntity() != null ? response.getEntity().getContent()
-                                                                        : InputStream.nullInputStream();
-                        if (response.getCode() >= HttpStatus.SC_REDIRECTION)
-                            throw readException(body.readAllBytes());
-
-                        return mapper.apply(new ForwardingInputStream(body) {
-                            @Override
-                            public void close() throws IOException {
-                                super.close();
-                                response.close();
+            return handle(response -> {
+                            try {
+                                InputStream body = response.getEntity() != null ? response.getEntity().getContent()
+                                                                                : InputStream.nullInputStream();
+                                if (response.getCode() >= HttpStatus.SC_REDIRECTION)
+                                    throw readException(body.readAllBytes());
+
+                                return mapper.apply(new ForwardingInputStream(body) {
+                                    @Override
+                                    public void close() throws IOException {
+                                        super.close();
+                                        response.close();
+                                    }
+                                });
                             }
-                        });
-                    }
-                    catch (IOException | RuntimeException | Error e) {
-                        try {
-                            response.close();
-                        }
-                        catch (IOException f) {
-                            e.addSuppressed(f);
-                        }
-                        if (e instanceof IOException)
-                            ioException = (IOException) e;
-                        else
-                            sneakyThrow(e);
-                    }
-                }
-                throw new RetryException(ioException);
-            });
+                            catch (IOException | RuntimeException | Error e) {
+                                try {
+                                    response.close();
+                                }
+                                catch (IOException f) {
+                                    e.addSuppressed(f);
+                                }
+                                if (e instanceof IOException)
+                                    throw new RetryException((IOException) e);
+                                else
+                                    sneakyThrow(e);
+                                throw new AssertionError("Should not happen");
+                            }
+                          },
+                          ioException -> {
+                              throw new RetryException(ioException);
+                          });
         }
 
     }
diff --git a/configserver-client/src/main/java/ai/vespa/hosted/client/ConfigServerClient.java b/configserver-client/src/main/java/ai/vespa/hosted/client/ConfigServerClient.java
index 234dbe9ee06..61b7edea0cc 100644
--- a/configserver-client/src/main/java/ai/vespa/hosted/client/ConfigServerClient.java
+++ b/configserver-client/src/main/java/ai/vespa/hosted/client/ConfigServerClient.java
@@ -12,6 +12,7 @@ import java.io.UncheckedIOException;
 import java.net.URI;
 import java.time.Duration;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 import java.util.function.BiFunction;
@@ -33,7 +34,10 @@ public interface ConfigServerClient extends AutoCloseable {
     interface RequestBuilder {
 
         /** Sets the request path. */
-        RequestBuilder at(String... pathSegments);
+        default RequestBuilder at(String... pathSegments) { return at(List.of(pathSegments)); }
+
+        /** Sets the request path. */
+        RequestBuilder at(List<String> pathSegments);
 
         /** Sets the request body as UTF-8 application/json. */
         RequestBuilder body(byte[] json);
@@ -42,24 +46,29 @@ public interface ConfigServerClient extends AutoCloseable {
         RequestBuilder body(HttpEntity entity);
 
         /** Sets the parameter key/values for the request. Number of arguments must be even. */
-        RequestBuilder parameters(String... pairs);
+        default RequestBuilder parameters(String... pairs) {
+            return parameters(Arrays.asList(pairs));
+        }
+
+        /** Sets the parameter key/values for the request. Number of arguments must be even. */
+        RequestBuilder parameters(List<String> pairs);
 
         /** Overrides the default socket read timeout of the request. {@code Duration.ZERO} gives infinite timeout. */
         RequestBuilder timeout(Duration timeout);
 
-        /** Overrides the default socket read timeout of the request. {@code null} allows infinite timeout. */
+        /** Overrides the default request config of the request. */
         RequestBuilder config(RequestConfig config);
 
         /**
          * Sets custom retry/failure logic for this.
          * <p>
-         * Exactly one of the arguments (response, exception) are non-null.
+         * Exactly one of the callbacks will be invoked, with a non-null argument.
          * Return a value to have that returned to the caller;
          * throw a {@link RetryException} to have the request retried; or
          * throw any other unchecked exception to have this propagate out to the caller.
          * The caller must close the provided response, if any.
          */
-        <T> T handle(BiFunction<ClassicHttpResponse, IOException, T> handler) throws UncheckedIOException;
+        <T> T handle(Function<ClassicHttpResponse, T> handler, Function<IOException, T> catcher) throws UncheckedIOException;
 
         /** Sets the response body mapper for this, for successful requests. */
         <T> T read(Function<byte[], T> mapper) throws UncheckedIOException, ConfigServerException;