More generic error response exception

3 files changed, 29 insertions, 61 deletions

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 e11f0db7194..0162b3b7a0f 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
@@ -1,8 +1,6 @@
 // Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package ai.vespa.hosted.client;
 
-import com.yahoo.slime.Inspector;
-import com.yahoo.slime.SlimeUtils;
 import org.apache.hc.client5.http.classic.methods.ClassicHttpRequests;
 import org.apache.hc.client5.http.config.RequestConfig;
 import org.apache.hc.client5.http.protocol.HttpClientContext;
@@ -25,12 +23,10 @@ import java.time.Duration;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.concurrent.TimeUnit;
-import java.util.function.BiFunction;
 import java.util.function.Function;
 import java.util.logging.Logger;
-import java.util.stream.Stream;
 
-import static ai.vespa.hosted.client.ConfigServerClient.ConfigServerException.ErrorCode.INCOMPLETE_RESPONSE;
+import static java.nio.charset.StandardCharsets.UTF_8;
 import static java.util.Objects.requireNonNull;
 import static java.util.logging.Level.FINE;
 import static java.util.logging.Level.WARNING;
@@ -180,7 +176,7 @@ public abstract class AbstractConfigServerClient implements ConfigServerClient {
         }
 
         @Override
-        public <T> T read(Function<byte[], T> mapper) throws UncheckedIOException, ConfigServerException {
+        public <T> T read(Function<byte[], T> mapper) throws UncheckedIOException, ResponseException {
             return mapIfSuccess(input -> {
                 try (input) {
                     return mapper.apply(input.readAllBytes());
@@ -192,7 +188,7 @@ public abstract class AbstractConfigServerClient implements ConfigServerClient {
         }
 
         @Override
-        public void discard() throws UncheckedIOException, ConfigServerException {
+        public void discard() throws UncheckedIOException, ResponseException {
             mapIfSuccess(input -> {
                 try (input) {
                     return null;
@@ -204,7 +200,7 @@ public abstract class AbstractConfigServerClient implements ConfigServerClient {
         }
 
         @Override
-        public InputStream stream() throws UncheckedIOException, ConfigServerException {
+        public InputStream stream() throws UncheckedIOException, ResponseException {
             return mapIfSuccess(input -> input);
         }
 
@@ -215,7 +211,7 @@ public abstract class AbstractConfigServerClient implements ConfigServerClient {
                                 InputStream body = response.getEntity() != null ? response.getEntity().getContent()
                                                                                 : InputStream.nullInputStream();
                                 if (response.getCode() >= HttpStatus.SC_REDIRECTION)
-                                    throw readException(body.readAllBytes());
+                                    throw new ResponseException(response.getCode(), new String(body.readAllBytes(), UTF_8), "");
 
                                 return mapper.apply(new ForwardingInputStream(body) {
                                     @Override
@@ -235,7 +231,7 @@ public abstract class AbstractConfigServerClient implements ConfigServerClient {
                                 if (e instanceof IOException)
                                     throw new RetryException((IOException) e);
                                 else
-                                    sneakyThrow(e);
+                                    sneakyThrow(e); // e is a runtime exception or an error, so this is fine.
                                 throw new AssertionError("Should not happen");
                             }
                           },
@@ -251,14 +247,4 @@ public abstract class AbstractConfigServerClient implements ConfigServerClient {
         throw (T) t;
     }
 
-    private static ConfigServerException readException(byte[] serialised) {
-        Inspector root = SlimeUtils.jsonToSlime(serialised).get();
-        String codeName = root.field("error-code").asString();
-        ConfigServerException.ErrorCode code = Stream.of(ConfigServerException.ErrorCode.values())
-                                                     .filter(value -> value.name().equals(codeName))
-                                                     .findAny().orElse(INCOMPLETE_RESPONSE);
-        String message = root.field("message").valid() ? root.field("message").asString() : "(no message)";
-        return new ConfigServerException(code, message, "");
-    }
-
 }
\ No newline at end of file
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 61b7edea0cc..3b24f1a568e 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
@@ -15,7 +15,6 @@ import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
-import java.util.function.BiFunction;
 import java.util.function.Function;
 import java.util.stream.IntStream;
 
@@ -71,13 +70,13 @@ public interface ConfigServerClient extends AutoCloseable {
         <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;
+        <T> T read(Function<byte[], T> mapper) throws UncheckedIOException, ResponseException;
 
         /** Discards the response, but throws if the response is unsuccessful. */
-        void discard() throws UncheckedIOException, ConfigServerException;
+        void discard() throws UncheckedIOException, ResponseException;
 
         /** Returns the raw input stream of the response, if successful. The caller must close the returned stream. */
-        InputStream stream() throws UncheckedIOException, ConfigServerException;
+        InputStream stream() throws UncheckedIOException, ResponseException;
 
     }
 
@@ -119,38 +118,21 @@ public interface ConfigServerClient extends AutoCloseable {
 
     }
 
-    /** An exception due to server error, a bad request, or similar. */
-    class ConfigServerException extends RuntimeException {
+    /** An exception due to server error, a bad request, or similar, which resulted in a non-OK HTTP response. */
+    class ResponseException extends RuntimeException {
 
-        private final ErrorCode errorId;
-        private final String message;
+        private final int code;
+        private final String body;
 
-        public ConfigServerException(ErrorCode errorId, String message, String context) {
-            super(context + ": " + message);
-            this.errorId = errorId;
-            this.message = message;
+        public ResponseException(int code, String body, String context) {
+            super(context + ": " + body);
+            this.code = code;
+            this.body = body;
         }
 
-        public ErrorCode errorId() { return errorId; }
-
-        public String message() { return message; }
-
-        public enum ErrorCode {
-            APPLICATION_LOCK_FAILURE,
-            BAD_REQUEST,
-            ACTIVATION_CONFLICT,
-            INTERNAL_SERVER_ERROR,
-            INVALID_APPLICATION_PACKAGE,
-            METHOD_NOT_ALLOWED,
-            NOT_FOUND,
-            OUT_OF_CAPACITY,
-            REQUEST_TIMEOUT,
-            UNKNOWN_VESPA_VERSION,
-            PARENT_HOST_NOT_READY,
-            CERTIFICATE_NOT_READY,
-            LOAD_BALANCER_NOT_READY,
-            INCOMPLETE_RESPONSE
-        }
+        public int code() { return code; }
+
+        public String body() { return body; }
 
     }
 
diff --git a/configserver-client/src/test/java/ai/vespa/hosted/client/HttpConfigServerClientTest.java b/configserver-client/src/test/java/ai/vespa/hosted/client/HttpConfigServerClientTest.java
index cbf38f46f6f..dffc94e94c0 100644
--- a/configserver-client/src/test/java/ai/vespa/hosted/client/HttpConfigServerClientTest.java
+++ b/configserver-client/src/test/java/ai/vespa/hosted/client/HttpConfigServerClientTest.java
@@ -1,7 +1,7 @@
 // Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package ai.vespa.hosted.client;
 
-import ai.vespa.hosted.client.ConfigServerClient.ConfigServerException;
+import ai.vespa.hosted.client.ConfigServerClient.ResponseException;
 import ai.vespa.hosted.client.ConfigServerClient.HostStrategy;
 import com.github.tomakehurst.wiremock.http.Fault;
 import com.yahoo.vespa.athenz.api.AthenzService;
@@ -81,15 +81,15 @@ class HttpConfigServerClientTest {
         server.verify(1, anyRequestedFor(anyUrl()));
         server.resetRequests();
 
-        // ConfigServerException is not retried.
+        // ResponseException is not retried.
         server.stubFor(get("/"))
-              .setResponse(aResponse().withStatus(409).withBody("{\"error-code\":\"ACTIVATION_CONFLICT\",\"message\":\"hi\"}").build());
-        ConfigServerException thrown = assertThrows(ConfigServerException.class,
-                                                    () -> client.send(HostStrategy.repeating(URI.create("http://localhost:" + server.port()), 10),
-                                                                      Method.GET)
-                                                                .read(String::new));
-        assertEquals(ConfigServerException.ErrorCode.ACTIVATION_CONFLICT, thrown.errorId());
-        assertEquals("hi", thrown.message());
+              .setResponse(aResponse().withStatus(409).withBody("hi").build());
+        ResponseException thrown = assertThrows(ResponseException.class,
+                                                () -> client.send(HostStrategy.repeating(URI.create("http://localhost:" + server.port()), 10),
+                                                                  Method.GET)
+                                                            .read(String::new));
+        assertEquals(409, thrown.code());
+        assertEquals("hi", thrown.body());
         server.verify(1, getRequestedFor(urlEqualTo("/")));
         server.verify(1, anyRequestedFor(anyUrl()));