diff options
author | Bjørn Christian Seime <bjorn.christian@seime.no> | 2024-02-27 10:25:46 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2024-02-27 10:25:46 +0100 |
commit | cd532e70ef3c4bb209ad52834e5d43249ff0295d (patch) | |
tree | f9271d8e6c7dc9b0c1eabd4335fcfc290b2edead | |
parent | 5c30c710f78047d565a24eca5683ce2fae18d3da (diff) | |
parent | fd8a182a634feb18dedd07a3868b701af0b6cb16 (diff) |
Merge pull request #30414 from vespa-engine/bjorncs/restapi
Handle `InvalidJsonException` by default. Fix mapper ordering
5 files changed, 61 insertions, 5 deletions
diff --git a/container-core/src/main/java/com/yahoo/restapi/RestApiImpl.java b/container-core/src/main/java/com/yahoo/restapi/RestApiImpl.java index 090e06c221f..a381fd3c9a6 100644 --- a/container-core/src/main/java/com/yahoo/restapi/RestApiImpl.java +++ b/container-core/src/main/java/com/yahoo/restapi/RestApiImpl.java @@ -172,7 +172,9 @@ class RestApiImpl implements RestApi { log.log(Level.FINE, e, e::getMessage); ExceptionMapperHolder<?> mapper = exceptionMappers.stream() .filter(holder -> holder.type.isAssignableFrom(e.getClass())) - .findFirst().orElseThrow(() -> e); + // Topologically sort children before superclasses, so most the specific match is found by iterating through mappers in order. + .min((a, b) -> (a.type.isAssignableFrom(b.type) ? 1 : 0) + (b.type.isAssignableFrom(a.type) ? -1 : 0)) + .orElseThrow(() -> e); return mapper.toResponse(context, e); } @@ -210,8 +212,6 @@ class RestApiImpl implements RestApi { if (!disableDefaultMappers){ exceptionMappers.addAll(RestApiMappers.DEFAULT_EXCEPTION_MAPPERS); } - // Topologically sort children before superclasses, so most the specific match is found by iterating through mappers in order. - exceptionMappers.sort((a, b) -> (a.type.isAssignableFrom(b.type) ? 1 : 0) + (b.type.isAssignableFrom(a.type) ? -1 : 0)); return exceptionMappers; } diff --git a/container-core/src/main/java/com/yahoo/restapi/RestApiMappers.java b/container-core/src/main/java/com/yahoo/restapi/RestApiMappers.java index 62b46d26ba9..6d785ba58bc 100644 --- a/container-core/src/main/java/com/yahoo/restapi/RestApiMappers.java +++ b/container-core/src/main/java/com/yahoo/restapi/RestApiMappers.java @@ -1,6 +1,7 @@ // Copyright Vespa.ai. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.restapi; +import ai.vespa.json.InvalidJsonException; import ai.vespa.json.Json; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; @@ -52,6 +53,7 @@ public class RestApiMappers { (context, entity) -> new JacksonJsonResponse<>(200, entity, context.jacksonJsonMapper(), true))); static List<ExceptionMapperHolder<?>> DEFAULT_EXCEPTION_MAPPERS = List.of( + new ExceptionMapperHolder<>(InvalidJsonException.class, (ctx, e) -> ErrorResponse.badRequest(e.getMessage())), new ExceptionMapperHolder<>(RestApiException.class, (context, exception) -> exception.response())); private RestApiMappers() {} diff --git a/container-core/src/test/java/com/yahoo/restapi/RestApiImplTest.java b/container-core/src/test/java/com/yahoo/restapi/RestApiImplTest.java index d27e04bbd7a..d959344685a 100644 --- a/container-core/src/test/java/com/yahoo/restapi/RestApiImplTest.java +++ b/container-core/src/test/java/com/yahoo/restapi/RestApiImplTest.java @@ -1,6 +1,7 @@ // Copyright Vespa.ai. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.restapi; +import ai.vespa.json.InvalidJsonException; import com.fasterxml.jackson.annotation.JsonProperty; import com.yahoo.container.jdisc.AclMapping; import com.yahoo.container.jdisc.HttpRequestBuilder; @@ -20,6 +21,8 @@ import java.time.Instant; import java.util.ArrayList; import java.util.List; import java.util.Map; +import java.util.MissingFormatWidthException; +import java.util.NoSuchElementException; import java.util.Set; import static com.yahoo.jdisc.http.HttpRequest.Method; @@ -99,6 +102,23 @@ class RestApiImplTest { } @Test + void chooses_most_specific_exception_mapper() { + RestApi restApi = RestApi.builder() + .addRoute(route("/json").get(ctx -> { throw new InvalidJsonException("oops invalid json"); })) + .addRoute(route("/illegal-argument").get(ctx -> { throw new IllegalArgumentException(); })) + .addRoute(route("/bad-format").get(ctx -> { throw new MissingFormatWidthException(""); })) + .addExceptionMapper(IllegalArgumentException.class, (ctx, exception) -> ErrorResponse.badRequest("oops illegal argument")) + .addExceptionMapper(NoSuchElementException.class, (ctx, exception) -> ErrorResponse.badRequest("oops no such element")) + .addExceptionMapper(RuntimeException.class, (ctx, exception) -> ErrorResponse.internalServerError("oops runtime")) + .addExceptionMapper(MissingFormatWidthException.class, (ctx, exception) -> ErrorResponse.internalServerError("oops bad format width")) + .build(); + // Uses default mapper for `InvalidJsonException` since it's more specific than `IllegalArgumentException` + verifyJsonResponse(restApi, Method.GET, "/json", null, 400, "{\"error-code\":\"BAD_REQUEST\",\"message\":\"oops invalid json\"}"); + verifyJsonResponse(restApi, Method.GET, "/illegal-argument", null, 400, "{\"message\":\"oops illegal argument\", \"error-code\":\"BAD_REQUEST\"}"); + verifyJsonResponse(restApi, Method.GET, "/bad-format", null, 500, "{\"message\":\"oops bad format width\", \"error-code\":\"INTERNAL_SERVER_ERROR\"}"); + } + + @Test void method_handler_can_consume_and_produce_json() { RestApi.HandlerWithRequestEntity<TestEntity, TestEntity> handler = (context, requestEntity) -> requestEntity; RestApi restApi = RestApi.builder() diff --git a/vespajlib/src/main/java/ai/vespa/json/Json.java b/vespajlib/src/main/java/ai/vespa/json/Json.java index b88c804c728..da7aae06e8d 100644 --- a/vespajlib/src/main/java/ai/vespa/json/Json.java +++ b/vespajlib/src/main/java/ai/vespa/json/Json.java @@ -8,6 +8,8 @@ import com.yahoo.slime.SlimeUtils; import com.yahoo.slime.Type; import java.math.BigDecimal; +import java.time.Instant; +import java.time.format.DateTimeParseException; import java.util.ArrayList; import java.util.Arrays; import java.util.Iterator; @@ -90,6 +92,20 @@ public class Json implements Iterable<Json> { return asBool(); } + public Optional<Instant> asOptionalInstant() { return isMissing() ? Optional.empty() : Optional.of(asInstant()); } + public Instant asInstant() { + requireType(Type.STRING); + try { + return Instant.parse(asString()); + } catch (DateTimeParseException e) { + throw new InvalidJsonException("Expected JSON member '%s' to be a valid timestamp: %s".formatted(path, e.getMessage())); + } + } + public Instant asInstant(Instant defaultValue) { + if (isMissing()) return defaultValue; + return asInstant(); + } + public List<Json> toList() { var list = new ArrayList<Json>(length()); forEachEntry(json -> list.add(json)); @@ -184,7 +200,14 @@ public class Json implements Iterable<Json> { public static Builder.Array newArray() { return new Builder.Array(new Slime().setArray()); } public static Builder.Object newObject() { return new Builder.Object(new Slime().setObject()); } - public static Builder.Object existingObject(Cursor cursor) { return new Builder.Object(cursor); } + public static Builder.Object existingSlimeObjectCursor(Cursor cursor) { + if (cursor.type() != Type.OBJECT) throw new InvalidJsonException("Input is not an object"); + return new Builder.Object(cursor); + } + public static Builder.Array existingSlimeArrayCursor(Cursor cursor) { + if (cursor.type() != Type.ARRAY) throw new InvalidJsonException("Input is not an array"); + return new Builder.Array(cursor); + } private Builder(Cursor cursor) { this.cursor = cursor; } @@ -200,6 +223,8 @@ public class Json implements Iterable<Json> { public Builder.Array add(Json json) { SlimeUtils.addValue(json.inspector, cursor.addObject()); return this; } + public Builder.Array add(Json.Builder builder) { return add(builder.build()); } + /** Note: does not return {@code this}! */ public Builder.Array addArray() { return new Array(cursor.addArray()); } /** Note: does not return {@code this}! */ @@ -223,6 +248,9 @@ public class Json implements Iterable<Json> { public Builder.Object set(String field, Json json) { SlimeUtils.setObjectEntry(json.inspector, field, cursor); return this; } + public Builder.Object set(String field, Json.Builder json) { + SlimeUtils.setObjectEntry(json.build().inspector, field, cursor); return this; + } /** Note: does not return {@code this}! */ public Builder.Array setArray(String field) { return new Array(cursor.setArray(field)); } /** Note: does not return {@code this}! */ @@ -233,6 +261,7 @@ public class Json implements Iterable<Json> { public Builder.Object set(String field, double value) { cursor.setDouble(field, value); return this; } public Builder.Object set(String field, boolean value) { cursor.setBool(field, value); return this; } public Builder.Object set(String field, BigDecimal value) { cursor.setString(field, value.toPlainString()); return this; } + public Builder.Object set(String field, Instant timestamp) { cursor.setString(field, timestamp.toString()); return this; } } public Cursor slimeCursor() { return cursor; } diff --git a/vespajlib/src/test/java/ai/vespa/json/JsonTest.java b/vespajlib/src/test/java/ai/vespa/json/JsonTest.java index 293e99227a7..51b64637fd8 100644 --- a/vespajlib/src/test/java/ai/vespa/json/JsonTest.java +++ b/vespajlib/src/test/java/ai/vespa/json/JsonTest.java @@ -23,7 +23,8 @@ class JsonTest { "array": [1, 2, 3], "quux": { "corge": "grault" - } + }, + "timestamp": "2021-06-01T12:00:00Z" } """; var json = Json.of(text); @@ -37,6 +38,7 @@ class JsonTest { assertEquals(8.25D, json.f("floaty").asDouble()); assertEquals(8L, json.f("floaty").asLong()); assertTrue(json.f("bool").asBool()); + assertEquals("2021-06-01T12:00:00Z", json.f("timestamp").asInstant().toString()); // Array member assertEquals(3, json.f("array").length()); @@ -78,6 +80,9 @@ class JsonTest { exception = assertThrows(InvalidJsonException.class, () -> json.f("string").asLong()); assertEquals("Expected JSON member 'string' to be a 'integer' or 'float' but got 'string'", exception.getMessage()); + + exception = assertThrows(InvalidJsonException.class, () -> json.f("string").asInstant()); + assertEquals("Expected JSON member 'string' to be a valid timestamp: Text 'bar' could not be parsed at index 0", exception.getMessage()); } @Test |