diff --git a/pom.xml b/pom.xml index c723676f0a..a751044b82 100644 --- a/pom.xml +++ b/pom.xml @@ -56,6 +56,14 @@ pom import + + + com.fasterxml.jackson.datatype + jackson-datatype-jdk8 + ${jackson.version} + pom + import + ${quarkus.platform.group-id} ${quarkus.platform.artifact-id} diff --git a/src/main/java/io/stargate/sgv2/jsonapi/exception/playing/APIException.java b/src/main/java/io/stargate/sgv2/jsonapi/exception/playing/APIException.java index 4e9d08f94d..374f7e51d6 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/exception/playing/APIException.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/exception/playing/APIException.java @@ -1,5 +1,7 @@ package io.stargate.sgv2.jsonapi.exception.playing; +import java.util.Objects; +import java.util.Optional; import java.util.UUID; import java.util.function.Supplier; @@ -40,6 +42,17 @@ public abstract class APIException extends RuntimeException implements Supplier { + // All errors default to 200 HTTP status code, because we have partial failure modes. + // There are some overrides, e.g. a server timeout may be a 500, this is managed in the + // error config. See ErrorTemplate. + public static final int DEFAULT_HTTP_RESPONSE = 200; + + /** + * HTTP Response code for this error. NOTE: Not using enum from quarkus because do not want + * references to the HTTP framework this deep into the command processing + */ + public final int httpResponse; + /** Unique identifier for this error instance. */ public final UUID errorId; @@ -71,17 +84,22 @@ public abstract class APIException extends RuntimeException public final String body; public APIException(ErrorInstance errorInstance) { + Objects.requireNonNull(errorInstance, "ErrorInstance cannot be null"); + this.errorId = errorInstance.errorId(); this.family = errorInstance.family(); this.scope = errorInstance.scope().scope(); this.code = errorInstance.code(); this.title = errorInstance.title(); this.body = errorInstance.body(); + Objects.requireNonNull( + errorInstance.httpResponseOverride(), "httpResponseOverride cannot be null"); + this.httpResponse = errorInstance.httpResponseOverride().orElse(DEFAULT_HTTP_RESPONSE); } public APIException( ErrorFamily family, ErrorScope scope, String code, String title, String body) { - this(new ErrorInstance(UUID.randomUUID(), family, scope, code, title, body)); + this(new ErrorInstance(UUID.randomUUID(), family, scope, code, title, body, Optional.empty())); } @Override diff --git a/src/main/java/io/stargate/sgv2/jsonapi/exception/playing/ErrorConfig.java b/src/main/java/io/stargate/sgv2/jsonapi/exception/playing/ErrorConfig.java index 0f1752493e..24ad7737c2 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/exception/playing/ErrorConfig.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/exception/playing/ErrorConfig.java @@ -6,6 +6,7 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.PropertyNamingStrategies; import com.fasterxml.jackson.dataformat.yaml.YAMLFactory; +import com.fasterxml.jackson.datatype.jdk8.Jdk8Module; import com.github.benmanes.caffeine.cache.Caffeine; import com.github.benmanes.caffeine.cache.LoadingCache; import com.google.common.annotations.VisibleForTesting; @@ -61,7 +62,7 @@ public static ErrorConfig getInstance() { private Map snippetVars; // Prefix used when adding snippets to the variables for a template. - private static final String SNIPPET_VAR_PREFIX = "SNIPPET."; + public static final String SNIPPET_VAR_PREFIX = "SNIPPET."; // TIDY: move this to the config sections public static final String DEFAULT_ERROR_CONFIG_FILE = "errors.yaml"; @@ -71,6 +72,7 @@ public ErrorConfig( @JsonProperty("snippets") List snippets, @JsonProperty("request_errors") List requestErrors, @JsonProperty("server_errors") List serverErrors) { + // defensive immutable copies because this config can be shared this.snippets = snippets == null ? List.of() : List.copyOf(snippets); this.requestErrors = requestErrors == null ? List.of() : List.copyOf(requestErrors); @@ -87,8 +89,19 @@ public ErrorConfig( * @param code * @param title * @param body + * @param httpResponseOverride Optional override for the HTTP response code for this error, only + * needs to be set if different from {@link APIException#DEFAULT_HTTP_RESPONSE}. NOTE: + * there is no checking that this is a well known HTTP status code, as we do not want to + * depend on classes like {@link jakarta.ws.rs.core.Response.Status} in this class and if we + * want to return a weird status this class should not limit that. It would be handled higher + * up the stack and tracked with Integration Tests. */ - public record ErrorDetail(String scope, String code, String title, String body) { + public record ErrorDetail( + String scope, + String code, + String title, + String body, + Optional httpResponseOverride) { public ErrorDetail { if (scope == null) { @@ -111,6 +124,8 @@ public record ErrorDetail(String scope, String code, String title, String body) if (body.isBlank()) { throw new IllegalArgumentException("body cannot be blank"); } + + Objects.requireNonNull(httpResponseOverride, "httpResponseOverride cannot be null"); } } @@ -133,6 +148,15 @@ public record Snippet(String name, String body) { throw new IllegalArgumentException("body cannot be blank"); } } + + /** + * Name to use for this snippet when substituting into templates. + * + * @return + */ + public String variableName() { + return SNIPPET_VAR_PREFIX + name; + } } /** @@ -199,8 +223,7 @@ public Map getSnippetVars() { // want the map to be immutable because we hand it out snippetVars = Map.copyOf( - snippets.stream() - .collect(Collectors.toMap(s -> SNIPPET_VAR_PREFIX + s.name(), Snippet::body))); + snippets.stream().collect(Collectors.toMap(s -> s.variableName(), Snippet::body))); } return snippetVars; } @@ -246,6 +269,9 @@ public static ErrorConfig readFromYamlString(String yaml) throws JsonProcessingE // This is only going to happen once at system start, ok to create a new mapper ObjectMapper mapper = new ObjectMapper(new YAMLFactory()); + // module for Optional support see + // https://github.com/FasterXML/jackson-modules-java8/tree/2.18/datatypes + mapper.registerModule(new Jdk8Module()); mapper.setPropertyNamingStrategy(PropertyNamingStrategies.SNAKE_CASE); return mapper.readValue(yaml, ErrorConfig.class); } diff --git a/src/main/java/io/stargate/sgv2/jsonapi/exception/playing/ErrorInstance.java b/src/main/java/io/stargate/sgv2/jsonapi/exception/playing/ErrorInstance.java index 51588e4234..201ea65a45 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/exception/playing/ErrorInstance.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/exception/playing/ErrorInstance.java @@ -1,5 +1,6 @@ package io.stargate.sgv2.jsonapi.exception.playing; +import java.util.Optional; import java.util.UUID; /** @@ -14,7 +15,8 @@ *

Will make it easier if / when we add more info to an error such as links to documentation. * They can be passed from the template through the ErrorInstance into to the APIException. * - *

+ *

NOTE: keeping variable checking out of this class for now, it's just a way to pass a + * single param rather than many. * * @param errorId * @param family @@ -22,6 +24,13 @@ * @param code * @param title * @param body + * @param httpResponseOverride */ public record ErrorInstance( - UUID errorId, ErrorFamily family, ErrorScope scope, String code, String title, String body) {} + UUID errorId, + ErrorFamily family, + ErrorScope scope, + String code, + String title, + String body, + Optional httpResponseOverride) {} diff --git a/src/main/java/io/stargate/sgv2/jsonapi/exception/playing/ErrorTemplate.java b/src/main/java/io/stargate/sgv2/jsonapi/exception/playing/ErrorTemplate.java index 472bd10baf..9422cfd52e 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/exception/playing/ErrorTemplate.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/exception/playing/ErrorTemplate.java @@ -4,6 +4,7 @@ import java.lang.reflect.InvocationTargetException; import java.util.HashMap; import java.util.Map; +import java.util.Optional; import java.util.UUID; import org.apache.commons.text.StringSubstitutor; @@ -38,6 +39,7 @@ * } * * + * @param The type of the {@link APIException} the template creates. * @param constructor A constructor accepts a single parameter of {@link ErrorInstance} and returns * an instance of the `T` type. * @param family {@link ErrorFamily} the error belongs to. @@ -46,7 +48,7 @@ * @param title Title of the error, does not change between instances. * @param messageTemplate A template for the error body, with variables to be replaced at runtime * using the {@link StringSubstitutor} from Apache Commons Text. - * @param The type of the {@link APIException} the template creates. + * @param httpResponseOverride If present, overrides the default HTTP 200 response code for errors. */ public record ErrorTemplate( Constructor constructor, @@ -54,7 +56,8 @@ public record ErrorTemplate( ErrorScope scope, String code, String title, - String messageTemplate) { + String messageTemplate, + Optional httpResponseOverride) { public T toException(Map values) { var errorInstance = toInstance(values); @@ -100,7 +103,8 @@ private ErrorInstance toInstance(Map values) { throw new UnresolvedErrorTemplateVariable(this, e.getMessage()); } - return new ErrorInstance(UUID.randomUUID(), family, scope, code, title, msg); + return new ErrorInstance( + UUID.randomUUID(), family, scope, code, title, msg, httpResponseOverride); } /** @@ -146,6 +150,12 @@ public static ErrorTemplate load( family, scope, code))); return new ErrorTemplate( - constructor, family, scope, code, errorConfig.title(), errorConfig.body()); + constructor, + family, + scope, + code, + errorConfig.title(), + errorConfig.body(), + errorConfig.httpResponseOverride()); } } diff --git a/src/main/resources/errors.yaml b/src/main/resources/errors.yaml index 19c8430fec..1dfa126858 100644 --- a/src/main/resources/errors.yaml +++ b/src/main/resources/errors.yaml @@ -34,6 +34,10 @@ # Each error object has: # - scope: UPPER_SNAKE_CASE_1 # - code: UPPER_SNAKE_CASE_1 +# - http_response_override: (optional) The HTTP status code to return when this error is thrown. If not present, the +# default status code is 200 for most things. This is not returned in the error object JSON +# It is used to override the HTTP status code in the response. +# NOTE: NO checking is done to confirm this is a valid HTTP status code. # - title: A short title for the error, that must not change between instances of the error. # - body: A longer body that may contain ${vars} to be passed by the code when created, and references to snippets. # This can be a multi line string, recommend using the `|-` to trim trailing newlines. diff --git a/src/test/java/io/stargate/sgv2/jsonapi/exception/playing/ErrorConfigTest.java b/src/test/java/io/stargate/sgv2/jsonapi/exception/playing/ErrorConfigTest.java index 74503517af..bebf76a61c 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/exception/playing/ErrorConfigTest.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/exception/playing/ErrorConfigTest.java @@ -6,6 +6,7 @@ import com.fasterxml.jackson.core.JsonProcessingException; import java.io.FileNotFoundException; +import java.util.Optional; import java.util.stream.Stream; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; @@ -44,6 +45,7 @@ public void readErrorsYaml() throws JsonProcessingException { - scope: TEST_SCOPE_1 code: TEST_ERROR_ID_1 title: the title for the error + http_response_override: 501 body: |- big long body with ${vars} in it - scope: @@ -70,6 +72,7 @@ public void readErrorsYaml() throws JsonProcessingException { assertThat(e.code()).isEqualTo("TEST_ERROR_ID_1"); assertThat(e.title()).isEqualTo("the title for the error"); assertThat(e.body()).isEqualTo("big long body with ${vars} in it"); + assertThat(e.httpResponseOverride()).isPresent().get().isEqualTo(501); }); assertThat(errorConfig.getErrorDetail(ErrorFamily.REQUEST, "", "TEST_ERROR_ID_2")) @@ -81,6 +84,7 @@ public void readErrorsYaml() throws JsonProcessingException { assertThat(e.code()).isEqualTo("TEST_ERROR_ID_2"); assertThat(e.title()).isEqualTo("This error has no scope"); assertThat(e.body()).isEqualTo("Line 1 of body\n\nLine 2 of body"); + assertThat(e.httpResponseOverride()).isEmpty(); }); assertThat(errorConfig.getErrorDetail(ErrorFamily.SERVER, "TEST_SCOPE_3", "TEST_ERROR_ID_2")) @@ -92,17 +96,26 @@ public void readErrorsYaml() throws JsonProcessingException { assertThat(e.code()).isEqualTo("TEST_ERROR_ID_2"); assertThat(e.title()).isEqualTo("the title for the error"); assertThat(e.body()).isEqualTo("big long body with ${vars} in it"); + assertThat(e.httpResponseOverride()).isEmpty(); }); } @ParameterizedTest @MethodSource("errorDetailTestCases") public void errorDetailValidation( - String scope, String code, String title, String body, Class errorClass, String message) { + String scope, + String code, + String title, + String body, + Optional httpStatusOverride, + Class errorClass, + String message) { T error = assertThrows( - errorClass, () -> new ErrorConfig.ErrorDetail(scope, code, title, body), "Error Type"); + errorClass, + () -> new ErrorConfig.ErrorDetail(scope, code, title, body, httpStatusOverride), + "Error Type"); assertThat(error.getMessage()).as("Error Message").isEqualTo(message); } @@ -114,25 +127,65 @@ private static Stream errorDetailTestCases() { "CODE", "title", "body", + Optional.empty(), IllegalArgumentException.class, "scope must be in UPPER_SNAKE_CASE_1 format, got: not_snake_scope"), Arguments.of( - "SCOPE", null, "title", "body", NullPointerException.class, "code cannot be null"), + "SCOPE", + null, + "title", + "body", + Optional.empty(), + NullPointerException.class, + "code cannot be null"), Arguments.of( "SCOPE", "not snake code", "title", "body", + Optional.empty(), IllegalArgumentException.class, "code must be in UPPER_SNAKE_CASE_1 format, got: not snake code"), Arguments.of( - "SCOPE", "CODE", null, "body", NullPointerException.class, "title cannot be null"), + "SCOPE", + "CODE", + null, + "body", + Optional.empty(), + NullPointerException.class, + "title cannot be null"), + Arguments.of( + "SCOPE", + "CODE", + "", + "body", + Optional.empty(), + IllegalArgumentException.class, + "title cannot be blank"), Arguments.of( - "SCOPE", "CODE", "", "body", IllegalArgumentException.class, "title cannot be blank"), + "SCOPE", + "CODE", + "title", + null, + Optional.empty(), + NullPointerException.class, + "body cannot be null"), Arguments.of( - "SCOPE", "CODE", "title", null, NullPointerException.class, "body cannot be null"), + "SCOPE", + "CODE", + "title", + "", + Optional.empty(), + IllegalArgumentException.class, + "body cannot be blank"), Arguments.of( - "SCOPE", "CODE", "title", "", IllegalArgumentException.class, "body cannot be blank")); + "SCOPE", + "CODE", + "title", + "body", + null, + NullPointerException.class, + "httpResponseOverride cannot be null")); } @Test @@ -160,6 +213,7 @@ public void readSnippetYaml() throws JsonProcessingException { s -> { assertThat(s.name()).isEqualTo("SNIPPET_1"); assertThat(s.body()).isEqualTo("Snippet 1 body"); + assertThat(errorConfig.getSnippetVars()).containsKey(s.variableName()); }); assertThat( errorConfig.snippets().stream().filter(e -> e.name().equals("SNIPPET_2")).findFirst()) @@ -169,6 +223,7 @@ public void readSnippetYaml() throws JsonProcessingException { s -> { assertThat(s.name()).isEqualTo("SNIPPET_2"); assertThat(s.body()).isEqualTo("Snippet 2 body\n\nmulti line"); + assertThat(errorConfig.getSnippetVars()).containsKey(s.variableName()); }); } diff --git a/src/test/java/io/stargate/sgv2/jsonapi/exception/playing/ErrorTemplateTest.java b/src/test/java/io/stargate/sgv2/jsonapi/exception/playing/ErrorTemplateTest.java index a51b65591d..c484f39d06 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/exception/playing/ErrorTemplateTest.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/exception/playing/ErrorTemplateTest.java @@ -57,6 +57,43 @@ private T createException(ErrorCode errorCode) { return error; } + /** + * Re-usable to Test various properties on an error + * + * @param error + * @param family + * @param scope + * @param code + * @param title + * @param httpResponseOverride + * @param + */ + private void assertError( + T error, + ErrorFamily family, + String scope, + String code, + String title, + Integer httpResponseOverride) { + // Does not accept a template because that would not catch the template being wrong + // pass in the values that error should have given the code etc. + + assertThat(error) + .isNotNull() + .satisfies( + e -> { + assertThat(e.family).isEqualTo(family); + assertThat(e.scope).isEqualTo(scope); + assertThat(e.code).isEqualTo(code); + assertThat(e.title).isEqualTo(title); + if (null == httpResponseOverride) { + assertThat(e.httpResponse).isEqualTo(APIException.DEFAULT_HTTP_RESPONSE); + } else { + assertThat(e.httpResponse).isEqualTo(httpResponseOverride); + } + }); + } + @Test public void scopedRequestError() { @@ -65,10 +102,13 @@ public void scopedRequestError() { .isNotNull() .satisfies( e -> { - assertThat(e.family).isEqualTo(ErrorFamily.REQUEST); - assertThat(e.scope).isEqualTo(TestRequestException.Scope.TEST_REQUEST_SCOPE.name()); - assertThat(e.code).isEqualTo(TestScopeException.Code.SCOPED_REQUEST_ERROR.name()); - assertThat(e.title).isEqualTo("A scoped request error"); + assertError( + e, + TestScopeException.FAMILY, + TestScopeException.SCOPE.name(), + TestScopeException.Code.SCOPED_REQUEST_ERROR.name(), + "A scoped request error", + null); assertThat(e.body) .contains( "long body with " @@ -87,10 +127,13 @@ public void unscopedRequestError() { .isNotNull() .satisfies( e -> { - assertThat(e.family).isEqualTo(ErrorFamily.REQUEST); - assertThat(e.scope).isBlank(); - assertThat(e.code).isEqualTo(TestRequestException.Code.UNSCOPED_REQUEST_ERROR.name()); - assertThat(e.title).isEqualTo("An unscoped request error"); + assertError( + e, + TestRequestException.FAMILY, + "", + TestRequestException.Code.UNSCOPED_REQUEST_ERROR.name(), + "An unscoped request error", + null); assertThat(e.body) .contains( "Multi line with " @@ -175,4 +218,22 @@ public void unknownErrorCode() { "Could not find error config for family=%s, scope=%s, code=%s", TestScopeException.FAMILY, TestScopeException.SCOPE, TEST_DATA.RANDOM_STRING)); } + + @Test + public void httpResponseOverride() { + var error = createException(TestRequestException.Code.HTTP_OVERRIDE); + + assertThat(error) + .isNotNull() + .satisfies( + e -> { + assertError( + e, + TestRequestException.FAMILY, + "", + TestRequestException.Code.HTTP_OVERRIDE.name(), + "An error that overrides the HTTP response", + 500); + }); + } } diff --git a/src/test/java/io/stargate/sgv2/jsonapi/exception/playing/MissingCtorException.java b/src/test/java/io/stargate/sgv2/jsonapi/exception/playing/MissingCtorException.java index 9c78784260..a0684ca833 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/exception/playing/MissingCtorException.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/exception/playing/MissingCtorException.java @@ -1,5 +1,7 @@ package io.stargate.sgv2.jsonapi.exception.playing; +import java.util.Optional; + /** * An exception that is missing the CTOR we need for the templating system. * @@ -14,7 +16,9 @@ public class MissingCtorException extends TestRequestException { // This ctor is just here to make it compile public MissingCtorException() { - super(new ErrorInstance(null, ErrorFamily.REQUEST, SCOPE, "FAKE", "title", "body")); + super( + new ErrorInstance( + null, ErrorFamily.REQUEST, SCOPE, "FAKE", "title", "body", Optional.empty())); } // This is the CTOR that is missing, kept commented out to show what is missing diff --git a/src/test/java/io/stargate/sgv2/jsonapi/exception/playing/TestRequestException.java b/src/test/java/io/stargate/sgv2/jsonapi/exception/playing/TestRequestException.java index 618e096391..e14c400c7d 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/exception/playing/TestRequestException.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/exception/playing/TestRequestException.java @@ -37,7 +37,8 @@ public String scope() { public enum Code implements ErrorCode { UNSCOPED_REQUEST_ERROR, - NO_VARIABLES_TEMPLATE; + NO_VARIABLES_TEMPLATE, + HTTP_OVERRIDE; private final ErrorTemplate template; diff --git a/src/test/resources/test_errors.yaml b/src/test/resources/test_errors.yaml index cee9024fef..0cc7fe021b 100644 --- a/src/test/resources/test_errors.yaml +++ b/src/test/resources/test_errors.yaml @@ -30,4 +30,11 @@ request_errors: code: NO_VARIABLES_TEMPLATE title: A body with no template variables body: |- - Body with no variables. \ No newline at end of file + Body with no variables. + + - scope: + code: HTTP_OVERRIDE + http_response_override: 500 + title: An error that overrides the HTTP response + body: |- + body with ${name} and ${value} in it \ No newline at end of file