diff --git a/UPGRADING.md b/UPGRADING.md index bbaae0fd60..e292a24a3a 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -3,7 +3,7 @@ Upgrading to Graylog 6.1.x ## Breaking Changes -- tbd +- API errors which are related to invalid requests (e.g. JSON parsing failures, invalid types, etc.) are now responded to with a type of `RequestError` instead of `APIError`. ## Java API Changes diff --git a/changelog/unreleased/pr-18367.toml b/changelog/unreleased/pr-18367.toml new file mode 100644 index 0000000000..69ebe2fcc6 --- /dev/null +++ b/changelog/unreleased/pr-18367.toml @@ -0,0 +1,4 @@ +type = "c" +message = "Improving API error responses for JSON parsing failures." + +pulls = ["19367"] diff --git a/data-node/src/main/java/org/graylog/datanode/initializers/JerseyService.java b/data-node/src/main/java/org/graylog/datanode/initializers/JerseyService.java index 1db5960727..54cc22ee05 100644 --- a/data-node/src/main/java/org/graylog/datanode/initializers/JerseyService.java +++ b/data-node/src/main/java/org/graylog/datanode/initializers/JerseyService.java @@ -53,7 +53,6 @@ import org.graylog.security.certutil.CertConstants; import org.graylog2.configuration.TLSProtocolsConfiguration; import org.graylog2.plugin.inject.Graylog2Module; import org.graylog2.rest.MoreMediaTypes; -import org.graylog2.shared.rest.exceptionmappers.JacksonPropertyExceptionMapper; import org.graylog2.shared.rest.exceptionmappers.JsonProcessingExceptionMapper; import org.graylog2.shared.security.tls.KeyStoreUtils; import org.slf4j.Logger; @@ -194,8 +193,7 @@ public class JerseyService extends AbstractIdleService { .registerClasses( JacksonXmlBindJsonProvider.class, JsonProcessingExceptionMapper.class, - JsonMappingExceptionMapper.class, - JacksonPropertyExceptionMapper.class) + JsonMappingExceptionMapper.class) // Replacing this with a lambda leads to missing subtypes - https://github.com/Graylog2/graylog2-server/pull/10617#discussion_r630236360 .register(new ContextResolver() { @Override diff --git a/full-backend-tests/src/test/java/org/graylog/exceptionmappers/JsonParsingErrorsIT.java b/full-backend-tests/src/test/java/org/graylog/exceptionmappers/JsonParsingErrorsIT.java new file mode 100644 index 0000000000..6fd721e920 --- /dev/null +++ b/full-backend-tests/src/test/java/org/graylog/exceptionmappers/JsonParsingErrorsIT.java @@ -0,0 +1,166 @@ +/* + * Copyright (C) 2020 Graylog, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the Server Side Public License, version 1, + * as published by MongoDB, Inc. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * Server Side Public License for more details. + * + * You should have received a copy of the Server Side Public License + * along with this program. If not, see + * . + */ +package org.graylog.exceptionmappers; + +import io.restassured.response.ValidatableResponse; +import org.graylog.testing.completebackend.Lifecycle; +import org.graylog.testing.completebackend.apis.GraylogApis; +import org.graylog.testing.containermatrix.SearchServer; +import org.graylog.testing.containermatrix.annotations.ContainerMatrixTest; +import org.graylog.testing.containermatrix.annotations.ContainerMatrixTestsConfiguration; + +import static io.restassured.RestAssured.given; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; + +@ContainerMatrixTestsConfiguration(serverLifecycle = Lifecycle.CLASS, searchVersions = SearchServer.OS2_LATEST) +public class JsonParsingErrorsIT { + private static final String SYNC_SEARCH = "/views/search/sync"; + private static final String STREAMS = "/streams"; + + private final GraylogApis api; + + public JsonParsingErrorsIT(GraylogApis api) { + this.api = api; + } + + @ContainerMatrixTest + void returnsSpecificErrorWhenTypeMismatches() { + assertErrorResponse(SYNC_SEARCH, """ + { + "queries": [ + { + "id": "75988e96-71e2-4f3f-9d14-d8e918571b16", + "query": { + "type": "elasticsearch", + "query_string": "" + }, + "timerange": { + "type": "relative", + "from": "foo" + } + } + ] + } + """) + .body("path", equalTo("queries.[0].timerange.from")) + .body("line", equalTo(11)) + .body("column", equalTo(14)) + .body("message", equalTo("Error at \"queries.[0].timerange.from\" [11, 14]: Must be of type int")) + .body("reference_path", equalTo( + "org.graylog.plugins.views.search.rest.AutoValue_SearchDTO$Builder[\"queries\"]" + + "->java.util.LinkedHashSet[0]->org.graylog.plugins.views.search.rest.AutoValue_QueryDTO$Builder[\"timerange\"]" + + "->org.graylog2.plugin.indexer.searches.timeranges.AutoValue_RelativeRange$Builder[\"from\"]")); + + } + + @ContainerMatrixTest + void returnsSpecificErrorForJsonParsingError() { + assertErrorResponse(SYNC_SEARCH, """ + { + "queries": [ + { + "id": "75988e96-71e2-4f3f-9d14-d8e918571b16", + "query": { + "type": "elasticsearch", + "query_string": "" + }, + "timerange": { + "type": "relative", + "from": 23, + } + } + ] + } + """) + .body("path", equalTo("queries.[0].timerange")) + .body("line", equalTo(12)) + .body("column", equalTo(5)) + .body("message", containsString("Unexpected character ('}' (code 125)): was expecting double-quote to start field name")) + .body("reference_path", equalTo( + "org.graylog.plugins.views.search.rest.AutoValue_SearchDTO$Builder[\"queries\"]" + + "->java.util.LinkedHashSet[0]" + + "->org.graylog.plugins.views.search.rest.AutoValue_QueryDTO$Builder[\"timerange\"]")); + + } + + @ContainerMatrixTest + void extractsReferencePathFromMissingProperty() { + assertErrorResponse(STREAMS, "{}") + .body("reference_path", equalTo("org.graylog2.rest.resources.streams.requests.CreateStreamRequest")); + + assertErrorResponse(STREAMS, """ + { + "title": "Foo", + "rules": [{}] + } + """) + .body("reference_path", equalTo( + "org.graylog2.rest.resources.streams.requests.CreateStreamRequest[\"rules\"]" + + "->java.util.ArrayList[0]" + + "->org.graylog2.rest.resources.streams.rules.requests.CreateStreamRuleRequest")); + } + + @ContainerMatrixTest + void handlesGenericJSONErrorsOnRootLevel() { + assertErrorResponse(STREAMS, """ + { + "title": "Foo", + } + """) + .body("message", equalTo("Unexpected character ('}' (code 125)): was expecting double-quote to start field name")) + .body("line", equalTo(3)) + .body("column", equalTo(1)); + + assertErrorResponse(STREAMS, """ + { + "title":: "Foo" + } + """) + .body("message", equalTo("Unexpected character (':' (code 58)): expected a valid value (JSON String, Number, Array, Object or token 'null', 'true' or 'false')")) + .body("line", equalTo(2)) + .body("column", equalTo(13)); + } + + @ContainerMatrixTest + void handleInvalidPropertiesOnRootLevel() { + assertErrorResponse(SYNC_SEARCH, """ + { + "foo": 23 + } + """) + .body("message", equalTo("Unable to map property foo.\nKnown properties include: parameters, id, queries")) + .body("line", equalTo(2)) + .body("column", equalTo(14)) + .body("path", equalTo("foo")) + .body("reference_path", equalTo("org.graylog.plugins.views.search.rest.AutoValue_SearchDTO$Builder[\"foo\"]")); + } + + private ValidatableResponse assertErrorResponse(String url, String body) { + return given() + .spec(api.requestSpecification()) + .log().ifValidationFails() + .when() + .body(body) + .post(url) + .then() + .log().ifValidationFails() + .assertThat() + .statusCode(400) + .body("type", equalTo("RequestError")); + } +} diff --git a/graylog2-server/src/main/java/org/graylog2/bootstrap/preflight/PreflightJerseyService.java b/graylog2-server/src/main/java/org/graylog2/bootstrap/preflight/PreflightJerseyService.java index 0edda084d6..1a032b52bd 100644 --- a/graylog2-server/src/main/java/org/graylog2/bootstrap/preflight/PreflightJerseyService.java +++ b/graylog2-server/src/main/java/org/graylog2/bootstrap/preflight/PreflightJerseyService.java @@ -25,6 +25,8 @@ import com.google.common.collect.ImmutableMap; import com.google.common.net.HostAndPort; import com.google.common.util.concurrent.AbstractIdleService; import com.google.common.util.concurrent.ThreadFactoryBuilder; +import jakarta.annotation.Nonnull; +import jakarta.inject.Inject; import jakarta.ws.rs.core.MediaType; import jakarta.ws.rs.ext.ContextResolver; import org.apache.commons.codec.digest.DigestUtils; @@ -42,14 +44,10 @@ import org.graylog2.bootstrap.preflight.web.BasicAuthFilter; import org.graylog2.configuration.HttpConfiguration; import org.graylog2.rest.MoreMediaTypes; import org.graylog2.shared.rest.exceptionmappers.AnyExceptionClassMapper; -import org.graylog2.shared.rest.exceptionmappers.JacksonPropertyExceptionMapper; import org.graylog2.shared.rest.exceptionmappers.JsonProcessingExceptionMapper; -import jakarta.annotation.Nonnull; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import jakarta.inject.Inject; - import java.net.URI; import java.util.Map; import java.util.Set; @@ -183,7 +181,6 @@ public class PreflightJerseyService extends AbstractIdleService { JacksonXmlBindJsonProvider.class, JsonProcessingExceptionMapper.class, JsonMappingExceptionMapper.class, - JacksonPropertyExceptionMapper.class, AnyExceptionClassMapper.class ) // Replacing this with a lambda leads to missing subtypes - https://github.com/Graylog2/graylog2-server/pull/10617#discussion_r630236360 diff --git a/graylog2-server/src/main/java/org/graylog2/plugin/rest/RequestError.java b/graylog2-server/src/main/java/org/graylog2/plugin/rest/RequestError.java new file mode 100644 index 0000000000..b5522bd80d --- /dev/null +++ b/graylog2-server/src/main/java/org/graylog2/plugin/rest/RequestError.java @@ -0,0 +1,48 @@ +/* + * Copyright (C) 2020 Graylog, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the Server Side Public License, version 1, + * as published by MongoDB, Inc. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * Server Side Public License for more details. + * + * You should have received a copy of the Server Side Public License + * along with this program. If not, see + * . + */ +package org.graylog2.plugin.rest; + +import com.fasterxml.jackson.annotation.JsonAutoDetect; +import com.fasterxml.jackson.annotation.JsonInclude; +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.annotation.JsonTypeName; +import com.google.auto.value.AutoValue; + +import javax.annotation.Nullable; +import java.util.Optional; + +@JsonAutoDetect +@AutoValue +@JsonTypeName("RequestError") // Explicitly indicates the class type to avoid AutoValue_ at the beginning +@JsonInclude(JsonInclude.Include.NON_EMPTY) +public abstract class RequestError implements GenericError { + @JsonProperty + public abstract int line(); + + @JsonProperty + public abstract int column(); + + @JsonProperty + public abstract String path(); + + @JsonProperty + public abstract Optional referencePath(); + + public static RequestError create(String message, int line, int column, String path, @Nullable String reference) { + return new AutoValue_RequestError(message, line, column, path, Optional.ofNullable(reference)); + } +} diff --git a/graylog2-server/src/main/java/org/graylog2/shared/initializers/JerseyService.java b/graylog2-server/src/main/java/org/graylog2/shared/initializers/JerseyService.java index 3b20828e03..b8b4df7f3c 100644 --- a/graylog2-server/src/main/java/org/graylog2/shared/initializers/JerseyService.java +++ b/graylog2-server/src/main/java/org/graylog2/shared/initializers/JerseyService.java @@ -67,7 +67,6 @@ import org.graylog2.shared.rest.VerboseCsrfProtectionFilter; import org.graylog2.shared.rest.XHRFilter; import org.graylog2.shared.rest.exceptionmappers.AnyExceptionClassMapper; import org.graylog2.shared.rest.exceptionmappers.BadRequestExceptionMapper; -import org.graylog2.shared.rest.exceptionmappers.JacksonPropertyExceptionMapper; import org.graylog2.shared.rest.exceptionmappers.JsonMappingExceptionMapper; import org.graylog2.shared.rest.exceptionmappers.JsonProcessingExceptionMapper; import org.graylog2.shared.rest.exceptionmappers.MissingStreamPermissionExceptionMapper; @@ -272,7 +271,6 @@ public class JerseyService extends AbstractIdleService { JacksonXmlBindJsonProvider.class, JsonProcessingExceptionMapper.class, JsonMappingExceptionMapper.class, - JacksonPropertyExceptionMapper.class, AnyExceptionClassMapper.class, MissingStreamPermissionExceptionMapper.class, WebApplicationExceptionMapper.class, diff --git a/graylog2-server/src/main/java/org/graylog2/shared/rest/exceptionmappers/JacksonPropertyExceptionMapper.java b/graylog2-server/src/main/java/org/graylog2/shared/rest/exceptionmappers/JacksonPropertyExceptionMapper.java deleted file mode 100644 index 6fc6e2fbf3..0000000000 --- a/graylog2-server/src/main/java/org/graylog2/shared/rest/exceptionmappers/JacksonPropertyExceptionMapper.java +++ /dev/null @@ -1,46 +0,0 @@ -/* - * Copyright (C) 2020 Graylog, Inc. - * - * This program is free software: you can redistribute it and/or modify - * it under the terms of the Server Side Public License, version 1, - * as published by MongoDB, Inc. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * Server Side Public License for more details. - * - * You should have received a copy of the Server Side Public License - * along with this program. If not, see - * . - */ -package org.graylog2.shared.rest.exceptionmappers; - -import com.fasterxml.jackson.databind.exc.PropertyBindingException; -import com.google.common.base.Joiner; -import org.graylog2.plugin.rest.ApiError; - -import jakarta.ws.rs.core.MediaType; -import jakarta.ws.rs.core.Response; -import jakarta.ws.rs.ext.ExceptionMapper; -import jakarta.ws.rs.ext.Provider; - -import java.util.Collection; -import java.util.Collections; - -import static com.google.common.base.MoreObjects.firstNonNull; -import static jakarta.ws.rs.core.Response.status; - -@Provider -public class JacksonPropertyExceptionMapper implements ExceptionMapper { - @Override - public Response toResponse(PropertyBindingException e) { - final Collection knownPropertyIds = firstNonNull(e.getKnownPropertyIds(), Collections.emptyList()); - final StringBuilder message = new StringBuilder("Unable to map property ") - .append(e.getPropertyName()) - .append(".\nKnown properties include: "); - Joiner.on(", ").appendTo(message, knownPropertyIds); - final ApiError apiError = ApiError.create(message.toString()); - return status(Response.Status.BAD_REQUEST).type(MediaType.APPLICATION_JSON_TYPE).entity(apiError).build(); - } -} diff --git a/graylog2-server/src/main/java/org/graylog2/shared/rest/exceptionmappers/JsonMappingExceptionMapper.java b/graylog2-server/src/main/java/org/graylog2/shared/rest/exceptionmappers/JsonMappingExceptionMapper.java index 9d6a9b60e5..0bc3b9eca4 100644 --- a/graylog2-server/src/main/java/org/graylog2/shared/rest/exceptionmappers/JsonMappingExceptionMapper.java +++ b/graylog2-server/src/main/java/org/graylog2/shared/rest/exceptionmappers/JsonMappingExceptionMapper.java @@ -17,12 +17,19 @@ package org.graylog2.shared.rest.exceptionmappers; import com.fasterxml.jackson.databind.JsonMappingException; -import org.graylog2.plugin.rest.ApiError; - +import com.fasterxml.jackson.databind.exc.MismatchedInputException; +import com.fasterxml.jackson.databind.exc.PropertyBindingException; +import com.fasterxml.jackson.databind.exc.ValueInstantiationException; +import com.google.common.base.Joiner; import jakarta.ws.rs.core.MediaType; import jakarta.ws.rs.core.Response; import jakarta.ws.rs.ext.ExceptionMapper; import jakarta.ws.rs.ext.Provider; +import org.graylog2.plugin.rest.RequestError; + +import java.util.Collection; +import java.util.Collections; +import java.util.stream.Collectors; import static com.google.common.base.MoreObjects.firstNonNull; import static jakarta.ws.rs.core.Response.status; @@ -31,8 +38,66 @@ import static jakarta.ws.rs.core.Response.status; public class JsonMappingExceptionMapper implements ExceptionMapper { @Override public Response toResponse(JsonMappingException e) { - final String message = firstNonNull(e.getMessage(), "Couldn't process JSON input"); - final ApiError apiError = ApiError.create(message); + final var errorPath = errorPath(e); + final var location = e.getLocation(); + final String message = errorWithJsonPath(e, errorPath); + final var referencePath = referencePath(e); + final var apiError = RequestError.create(message, location.getLineNr(), location.getColumnNr(), errorPath, referencePath); return status(Response.Status.BAD_REQUEST).type(MediaType.APPLICATION_JSON_TYPE).entity(apiError).build(); } + + private String referencePath(JsonMappingException e) { + final var sb = new StringBuilder().append(e.getPathReference()); + + if (e instanceof ValueInstantiationException vie) { + if (!sb.isEmpty()) { + sb.append("->"); + } + sb.append(vie.getType().getRawClass().getCanonicalName()); + } + + return sb.toString(); + } + + private String errorPath(final JsonMappingException e) { + return e.getPath().stream() + .map(path -> { + final var fieldName = path.getFieldName(); + if (fieldName == null && path.getIndex() != -1) { + return "[" + path.getIndex() + "]"; + } + return fieldName; + }) + .collect(Collectors.joining(".")); + } + + private String errorWithJsonPath(final JsonMappingException e, String path) { + final var location = "[" + e.getLocation().getLineNr() + ", " + e.getLocation().getColumnNr() + "]"; + final var quotedPath = "\"" + path + "\""; + final var messagePrefix = "Error at " + quotedPath + " " + location; + + + if (e instanceof PropertyBindingException propertyBindingException) { + final Collection knownPropertyIds = firstNonNull(propertyBindingException.getKnownPropertyIds(), Collections.emptyList()); + final StringBuilder message = new StringBuilder("Unable to map property ") + .append(propertyBindingException.getPropertyName()) + .append(".\nKnown properties include: "); + Joiner.on(", ").appendTo(message, knownPropertyIds); + return message.toString(); + } + if (e instanceof MismatchedInputException mismatchedInputException) { + final var targetType = mismatchedInputException.getTargetType(); + if (targetType != null) { + return messagePrefix + ": Must be of type " + mismatchedInputException.getTargetType().getSimpleName(); + } else { + return messagePrefix + ": " + mismatchedInputException.getMessage(); + } + } else { + final var cause = e.getCause(); + final String problemMessage = firstNonNull(cause, e).getMessage(); + final String message = firstNonNull(problemMessage, "Couldn't process JSON input"); + + return messagePrefix + ": " + message; + } + } } diff --git a/graylog2-server/src/main/java/org/graylog2/shared/rest/exceptionmappers/JsonProcessingExceptionMapper.java b/graylog2-server/src/main/java/org/graylog2/shared/rest/exceptionmappers/JsonProcessingExceptionMapper.java index 8f864b45f2..7ee82f9bd8 100644 --- a/graylog2-server/src/main/java/org/graylog2/shared/rest/exceptionmappers/JsonProcessingExceptionMapper.java +++ b/graylog2-server/src/main/java/org/graylog2/shared/rest/exceptionmappers/JsonProcessingExceptionMapper.java @@ -17,12 +17,11 @@ package org.graylog2.shared.rest.exceptionmappers; import com.fasterxml.jackson.core.JsonProcessingException; -import org.graylog2.plugin.rest.ApiError; - import jakarta.ws.rs.core.MediaType; import jakarta.ws.rs.core.Response; import jakarta.ws.rs.ext.ExceptionMapper; import jakarta.ws.rs.ext.Provider; +import org.graylog2.plugin.rest.RequestError; import static com.google.common.base.MoreObjects.firstNonNull; import static jakarta.ws.rs.core.Response.status; @@ -31,8 +30,9 @@ import static jakarta.ws.rs.core.Response.status; public class JsonProcessingExceptionMapper implements ExceptionMapper { @Override public Response toResponse(JsonProcessingException e) { - final String message = firstNonNull(e.getMessage(), "Couldn't process JSON input"); - final ApiError apiError = ApiError.create(message); + final String message = firstNonNull(e.getOriginalMessage(), "Couldn't process JSON input"); + final var location = e.getLocation(); + final var apiError = RequestError.create(message, location.getLineNr(), location.getColumnNr(), "", null); return status(Response.Status.BAD_REQUEST).type(MediaType.APPLICATION_JSON_TYPE).entity(apiError).build(); } } diff --git a/graylog2-server/src/test/java/org/graylog2/plugin/rest/exceptionmappers/JsonProcessingExceptionMapperTest.java b/graylog2-server/src/test/java/org/graylog2/plugin/rest/exceptionmappers/JsonProcessingExceptionMapperTest.java index 988b879763..991ac4ce4d 100644 --- a/graylog2-server/src/test/java/org/graylog2/plugin/rest/exceptionmappers/JsonProcessingExceptionMapperTest.java +++ b/graylog2-server/src/test/java/org/graylog2/plugin/rest/exceptionmappers/JsonProcessingExceptionMapperTest.java @@ -17,23 +17,21 @@ package org.graylog2.plugin.rest.exceptionmappers; import com.fasterxml.jackson.core.JsonFactory; -import com.fasterxml.jackson.core.JsonLocation; import com.fasterxml.jackson.core.JsonParser; import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.JsonMappingException; -import org.graylog2.plugin.rest.ApiError; +import jakarta.ws.rs.core.MediaType; +import jakarta.ws.rs.core.Response; +import jakarta.ws.rs.ext.ExceptionMapper; +import org.graylog2.plugin.rest.RequestError; import org.graylog2.shared.bindings.GuiceInjectorHolder; import org.graylog2.shared.rest.exceptionmappers.JsonProcessingExceptionMapper; import org.junit.BeforeClass; import org.junit.Test; -import jakarta.ws.rs.core.MediaType; -import jakarta.ws.rs.core.Response; -import jakarta.ws.rs.ext.ExceptionMapper; - import java.util.Collections; -import static org.junit.Assert.assertEquals; +import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.assertTrue; public class JsonProcessingExceptionMapperTest { @@ -49,12 +47,12 @@ public class JsonProcessingExceptionMapperTest { final JsonMappingException exception = new JsonMappingException(jsonParser, "Boom!", new RuntimeException("rootCause")); final Response response = mapper.toResponse(exception); - assertEquals(Response.Status.BAD_REQUEST, response.getStatusInfo()); - assertEquals(MediaType.APPLICATION_JSON_TYPE, response.getMediaType()); - assertTrue(response.hasEntity()); - assertTrue(response.getEntity() instanceof ApiError); + assertThat(response.getStatusInfo()).isEqualTo(Response.Status.BAD_REQUEST); + assertThat(response.getMediaType()).isEqualTo(MediaType.APPLICATION_JSON_TYPE); + assertThat(response.hasEntity()).isTrue(); + assertThat(response.getEntity()).isInstanceOf(RequestError.class); - final ApiError responseEntity = (ApiError) response.getEntity(); + final var responseEntity = (RequestError) response.getEntity(); assertTrue(responseEntity.message().startsWith("Boom!")); } }