From abcb7b59536d92b388af5e0dbafd050ad4e5f91d Mon Sep 17 00:00:00 2001 From: Jon Marius Venstad Date: Wed, 21 Apr 2021 15:52:31 +0200 Subject: More generic error response exception --- .../hosted/client/AbstractConfigServerClient.java | 26 +++--------- .../ai/vespa/hosted/client/ConfigServerClient.java | 46 +++++++--------------- .../hosted/client/HttpConfigServerClientTest.java | 18 ++++----- 3 files changed, 29 insertions(+), 61 deletions(-) (limited to 'configserver-client') 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 read(Function mapper) throws UncheckedIOException, ConfigServerException { + public T read(Function 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 handle(Function handler, Function catcher) throws UncheckedIOException; /** Sets the response body mapper for this, for successful requests. */ - T read(Function mapper) throws UncheckedIOException, ConfigServerException; + T read(Function 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())); -- cgit v1.2.3