Default WebExceptionMapper preserves exception HTTP headers (#1912)

* Preserves headers from exception in response

Use case: enables NotAllowedException to communicate the valid allowed
methods on the given endpoint via the "Allow" HTTP header.

* Update release notes

docs/source/about/release-notes.rst

@@ -18,6 +18,7 @@ v1.1.0: Unreleased
 * Allow overriding of a default ``ExceptionMapper`` without re-registering all other defaults `#1768 <https://github.com/dropwizard/dropwizard/pull/1768>`_
 * Allow overriding of default ``JsonProvider`` `#1788 <https://github.com/dropwizard/dropwizard/pull/1788>`_
 * Finer-grain control of exception behaviour in view renderers `#1820 <https://github.com/dropwizard/dropwizard/pull/1820>`_
+* Default ``WebApplicationException`` handler preserves exception HTTP headers `#1912 <https://github.com/dropwizard/dropwizard/pull/1912>`_
 * JerseyClientBuilder can create rx-capable client `#1721 <https://github.com/dropwizard/dropwizard/pull/1721>`_
 * Configurable response for empty `Optional` return values `#1784 <https://github.com/dropwizard/dropwizard/pull/1784>`_
 * Add web test container agnostic way of invoking requests in ``ResourceTestRule`` `#1778 <https://github.com/dropwizard/dropwizard/pull/1778>`_

dropwizard-jersey/src/main/java/io/dropwizard/jersey/errors/LoggingExceptionMapper.java

@@ -16,9 +16,8 @@ public abstract class LoggingExceptionMapper<E extends Throwable> implements Exc
 
     @Override
     public Response toResponse(E exception) {
-        final int status;
-        final ErrorMessage errorMessage;
-
+        // If we're dealing with a web exception, we can service certain types of request (like
+        // redirection or server errors) better and also propagate properties of the inner response.
         if (exception instanceof WebApplicationException) {
             final Response response = ((WebApplicationException) exception).getResponse();
             Response.Status.Family family = response.getStatusInfo().getFamily();
@@ -28,17 +27,20 @@ public Response toResponse(E exception) {
             if (family.equals(Response.Status.Family.SERVER_ERROR)) {
                 logException(exception);
             }
-            status = response.getStatus();
-            errorMessage = new ErrorMessage(status, exception.getLocalizedMessage());
-        } else {
-            final long id = logException(exception);
-            status = Response.Status.INTERNAL_SERVER_ERROR.getStatusCode();
-            errorMessage = new ErrorMessage(formatErrorMessage(id, exception));
+
+            return Response.fromResponse(response)
+                .type(MediaType.APPLICATION_JSON_TYPE)
+                .entity(new ErrorMessage(response.getStatus(), exception.getLocalizedMessage()))
+                .build();
         }
 
-        return Response.status(status)
+        // Else the thrown exception is a not a web exception, so the exception is most likely
+        // unexpected. We'll create a unique id in the server error response that is also logged for
+        // correlation
+        final long id = logException(exception);
+        return Response.status(Response.Status.INTERNAL_SERVER_ERROR.getStatusCode())
                 .type(MediaType.APPLICATION_JSON_TYPE)
-                .entity(errorMessage)
+                .entity(new ErrorMessage(formatErrorMessage(id, exception)))
                 .build();
     }
 

dropwizard-jersey/src/test/java/io/dropwizard/jersey/errors/LoggingExceptionMapperTest.java

@@ -1,16 +1,20 @@
 package io.dropwizard.jersey.errors;
 
 import com.codahale.metrics.MetricRegistry;
+import com.google.common.collect.ImmutableList;
 import io.dropwizard.jersey.AbstractJerseyTest;
 import io.dropwizard.jersey.DropwizardResourceConfig;
 import org.junit.Test;
 
 import javax.ws.rs.WebApplicationException;
+import javax.ws.rs.client.Entity;
 import javax.ws.rs.core.Application;
 import javax.ws.rs.core.MediaType;
 import javax.ws.rs.core.Response;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.catchThrowable;
+import static org.assertj.core.api.Assertions.entry;
 import static org.assertj.core.api.Assertions.failBecauseExceptionWasNotThrown;
 
 public class LoggingExceptionMapperTest extends AbstractJerseyTest {
@@ -51,6 +55,18 @@ public void handlesJsonMappingException() throws Exception {
         }
     }
 
+    @Test
+    public void handlesMethodNotAllowedWithHeaders() {
+        final Throwable thrown = catchThrowable(() -> target("/exception/json-mapping-exception")
+            .request(MediaType.APPLICATION_JSON)
+            .post(Entity.json("A"), String.class));
+        assertThat(thrown).isInstanceOf(WebApplicationException.class);
+        final Response resp = ((WebApplicationException) thrown).getResponse();
+        assertThat(resp.getStatus()).isEqualTo(405);
+        assertThat(resp.getHeaders()).contains(entry("Allow", ImmutableList.of("GET,OPTIONS")));
+        assertThat(resp.readEntity(String.class)).isEqualTo("{\"code\":405,\"message\":\"HTTP 405 Method Not Allowed\"}");
+    }
+
     @Test
     public void formatsWebApplicationException() throws Exception {
         try {