Addressed comments.

--Also moved the non-deprecated constructors of `ResponseException`
above the deprecated ones.

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)

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());
     }
 
     /**

fili-core/src/main/java/com/yahoo/bard/webservice/web/endpoints/JobsServlet.java

@@ -492,7 +492,9 @@ protected Observable<PreResponse> 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);
         }

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<ResultSetMapper> getMappers() {
     public DataApiRequest getDataApiRequest() {
         return apiRequest;
     }
+
+    protected ObjectMappersSuite getObjectMappers() {
+        return objectMappers;
+    }
 }

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()
+            ));
         }
     }
 

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(