diff --git a/CHANGELOG.md b/CHANGELOG.md index a237d32738..3419335b15 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,10 +55,13 @@ Current ### Deprecated: - +- [All constructors of `ResponseException` that do not take an `ObjectWriter`](https://github.com/yahoo/fili/pull/70) + * An `ObjectWriter` is required in order to ensure that the exception correctly serializes its associated Druid query ### Fixed: +- [Druid queries are now serialized correctly when logging `ResponseExceptions`](https://github.com/yahoo/fili/pull/70) + - [Fixed typo emit -> omit in Utils.java omitField()](https://github.com/yahoo/fili/pull/68) - [Adds read locking to all attempts to read the Lucene index](https://github.com/yahoo/fili/pull/52) diff --git a/fili-core/src/main/java/com/yahoo/bard/webservice/async/ResponseException.java b/fili-core/src/main/java/com/yahoo/bard/webservice/async/ResponseException.java index baf8d8d6c0..6eecced928 100644 --- a/fili-core/src/main/java/com/yahoo/bard/webservice/async/ResponseException.java +++ b/fili-core/src/main/java/com/yahoo/bard/webservice/async/ResponseException.java @@ -28,24 +28,35 @@ public class ResponseException extends Exception { private final Throwable cause; /** - * Class constructor with throwable and other parameters. + * Class constructor with all the parameters to prepare the error response, plus a writer to serialize the Druid + * query. * - * @param statusType Status type of the response + * @param statusCode Http status code for the error + * @param reason Reason for the error + * @param description Description for the error * @param druidQuery The druid query being processed - * @param error Exception object with error details + * @param cause Exception object with error details + * @param objectWriter The writer to use to serialize the Druid query */ - public ResponseException(Response.StatusType statusType, DruidAggregationQuery druidQuery, Throwable error) { - this( - statusType.getStatusCode(), - ErrorUtils.getReason(error), - ErrorUtils.getDescription(error), - druidQuery, - error - ); + public ResponseException( + int statusCode, + String reason, + String description, + DruidQuery druidQuery, + Throwable cause, + ObjectWriter objectWriter + ) { + super(buildMessage(reason, description, statusCode, druidQuery, cause, objectWriter)); + this.statusCode = statusCode; + this.reason = reason; + this.description = description; + this.druidQuery = druidQuery; + this.cause = cause; } /** * Class constructor with throwable, other parameters and a mapper for serializing the druid query. + * * @param statusType Status type of the response * @param druidQuery The druid query being processed * @param error Exception object with error details @@ -68,61 +79,60 @@ public ResponseException( } /** - * Class constructor with all the parameters to prepare the error response by considering error object as null. + * Class constructor with throwable and other parameters. * - * @param statusCode Http status code for the error - * @param reason Reason for the error - * @param description Description for the error + * @param statusType Status type of the response * @param druidQuery The druid query being processed + * @param error Exception object with error details + * @deprecated In order to ensure correct serialization of the Druid Query, an ObjectWriter with all appropriate + * configuration should be passed in to the constructor */ - public ResponseException(int statusCode, String reason, String description, DruidQuery druidQuery) { - this(statusCode, reason, description, druidQuery, null); + @Deprecated + public ResponseException(Response.StatusType statusType, DruidAggregationQuery druidQuery, Throwable error) { + this( + statusType.getStatusCode(), + ErrorUtils.getReason(error), + ErrorUtils.getDescription(error), + druidQuery, + error + ); } /** - * Class constructor with all the parameters to prepare the error response. + * Class constructor with all the parameters to prepare the error response by considering error object as null. * * @param statusCode Http status code for the error * @param reason Reason for the error * @param description Description for the error * @param druidQuery The druid query being processed - * @param cause Exception object with error details + * @deprecated In order to ensure correct serialization of the Druid Query, an ObjectWriter with all appropriate + * configuration should be passed in to the constructor */ - public ResponseException( - int statusCode, - String reason, - String description, - DruidQuery druidQuery, - Throwable cause - ) { - this(statusCode, reason, description, druidQuery, cause, new ObjectMapper().writer()); + @Deprecated + public ResponseException(int statusCode, String reason, String description, DruidQuery druidQuery) { + this(statusCode, reason, description, druidQuery, null); } /** - * Class constructor with all the parameters to prepare the error response, plus a writer to serialize the Druid - * query. + * Class constructor with all the parameters to prepare the error response. * * @param statusCode Http status code for the error * @param reason Reason for the error * @param description Description for the error * @param druidQuery The druid query being processed * @param cause Exception object with error details - * @param objectWriter The writer to use to serialize the Druid query + * @deprecated In order to ensure correct serialization of the Druid Query, an ObjectWriter with all appropriate + * configuration should be passed in to the constructor */ + @Deprecated public ResponseException( int statusCode, String reason, String description, DruidQuery druidQuery, - Throwable cause, - ObjectWriter objectWriter + Throwable cause ) { - super(buildMessage(reason, description, statusCode, druidQuery, cause, objectWriter)); - this.statusCode = statusCode; - this.reason = reason; - this.description = description; - this.druidQuery = druidQuery; - this.cause = cause; + this(statusCode, reason, description, druidQuery, cause, new ObjectMapper().writer()); } /** diff --git a/fili-core/src/main/java/com/yahoo/bard/webservice/web/endpoints/JobsServlet.java b/fili-core/src/main/java/com/yahoo/bard/webservice/web/endpoints/JobsServlet.java index e8a73a93f9..62368df035 100644 --- a/fili-core/src/main/java/com/yahoo/bard/webservice/web/endpoints/JobsServlet.java +++ b/fili-core/src/main/java/com/yahoo/bard/webservice/web/endpoints/JobsServlet.java @@ -492,7 +492,9 @@ protected Observable handlePreResponseWithError( (Integer) responseContext.get(ResponseContextKeys.STATUS.getName()), (String) responseContext.get(ResponseContextKeys.ERROR_MESSAGE.getName()), (String) responseContext.get(ResponseContextKeys.ERROR_MESSAGE.getName()), - null + null, // Druid query + null, // cause + writer ); return Observable.error(responseException); } diff --git a/fili-core/src/main/java/com/yahoo/bard/webservice/web/responseprocessors/MappingResponseProcessor.java b/fili-core/src/main/java/com/yahoo/bard/webservice/web/responseprocessors/MappingResponseProcessor.java index 337dd633e9..2d1ee8740d 100644 --- a/fili-core/src/main/java/com/yahoo/bard/webservice/web/responseprocessors/MappingResponseProcessor.java +++ b/fili-core/src/main/java/com/yahoo/bard/webservice/web/responseprocessors/MappingResponseProcessor.java @@ -108,7 +108,14 @@ public HttpErrorCallback getStandardError( @Override public void invoke(int statusCode, String reason, String responseBody) { LOG.error(ErrorMessageFormat.ERROR_FROM_DRUID.logFormat(responseBody, statusCode, reason, druidQuery)); - responseEmitter.onError(new ResponseException(statusCode, reason, responseBody, druidQuery)); + responseEmitter.onError(new ResponseException( + statusCode, + reason, + responseBody, + druidQuery, + null, + getObjectMappers().getMapper().writer() + )); } }; } @@ -155,4 +162,8 @@ public List getMappers() { public DataApiRequest getDataApiRequest() { return apiRequest; } + + protected ObjectMappersSuite getObjectMappers() { + return objectMappers; + } } diff --git a/fili-core/src/main/java/com/yahoo/bard/webservice/web/responseprocessors/ResultSetResponseProcessor.java b/fili-core/src/main/java/com/yahoo/bard/webservice/web/responseprocessors/ResultSetResponseProcessor.java index a4767de0b0..052a778942 100644 --- a/fili-core/src/main/java/com/yahoo/bard/webservice/web/responseprocessors/ResultSetResponseProcessor.java +++ b/fili-core/src/main/java/com/yahoo/bard/webservice/web/responseprocessors/ResultSetResponseProcessor.java @@ -118,10 +118,20 @@ public void processResponse(JsonNode json, DruidAggregationQuery druidQuery, responseEmitter.onCompleted(); } catch (PageNotFoundException invalidPage) { LOG.debug(invalidPage.getLogMessage()); - responseEmitter.onError(new ResponseException(invalidPage.getErrorStatus(), druidQuery, invalidPage)); + responseEmitter.onError(new ResponseException( + invalidPage.getErrorStatus(), + druidQuery, + invalidPage, + getObjectMappers().getMapper().writer() + )); } catch (Exception exception) { LOG.error("Exception processing druid call in success", exception); - responseEmitter.onError(new ResponseException(Status.INTERNAL_SERVER_ERROR, druidQuery, exception)); + responseEmitter.onError(new ResponseException( + Status.INTERNAL_SERVER_ERROR, + druidQuery, + exception, + getObjectMappers().getMapper().writer() + )); } } diff --git a/fili-core/src/test/groovy/com/yahoo/bard/webservice/async/workflows/DefaultAsynchronousWorkflowsBuilderSpec.groovy b/fili-core/src/test/groovy/com/yahoo/bard/webservice/async/workflows/DefaultAsynchronousWorkflowsBuilderSpec.groovy index 5af390d75f..c8bedfd0ac 100644 --- a/fili-core/src/test/groovy/com/yahoo/bard/webservice/async/workflows/DefaultAsynchronousWorkflowsBuilderSpec.groovy +++ b/fili-core/src/test/groovy/com/yahoo/bard/webservice/async/workflows/DefaultAsynchronousWorkflowsBuilderSpec.groovy @@ -20,6 +20,8 @@ import com.yahoo.bard.webservice.web.PreResponse import com.yahoo.bard.webservice.web.responseprocessors.ResponseContext import com.yahoo.bard.webservice.web.responseprocessors.ResponseContextKeys +import com.fasterxml.jackson.databind.ObjectMapper + import org.joda.time.DateTime import rx.Observable @@ -61,7 +63,9 @@ class DefaultAsynchronousWorkflowsBuilderSpec extends Specification { 404, "not found", "not found", - Mock(DruidAggregationQuery) + Mock(DruidAggregationQuery), + null, // cause + null // Not serializing the exception. ) @Subject DefaultAsynchronousWorkflowsBuilder asynchronousProcessBuilder = new DefaultAsynchronousWorkflowsBuilder(