Merge pull request #20230 from yrodiere/i20198-quarkuserrorhandler-de…

…faultcontenttype Rework "Accept" header handling for HTTP 500 response body on unhandled exceptions
quarkusio · Sep 21, 2021 · 26e88e6 · 26e88e6
2 parents e101a78 + e49ae01
commit 26e88e6
Show file tree

Hide file tree

Showing 16 changed files with 493 additions and 41 deletions.
diff --git a/...ive-routes/deployment/src/test/java/io/quarkus/vertx/web/cs/CompletionStageRouteTest.java b/...ive-routes/deployment/src/test/java/io/quarkus/vertx/web/cs/CompletionStageRouteTest.java
@@ -44,7 +44,7 @@ public void testCsRoute() {
         when().get("/failure").then().statusCode(500).body(containsString("boom"));
         when().get("/sync-failure").then().statusCode(500).body(containsString("boom"));
 
-        when().get("/null").then().statusCode(500).body(containsString("null"));
+        when().get("/null").then().statusCode(500).body(containsString(NullPointerException.class.getName()));
         when().get("/cs-null").then().statusCode(500);
         when().get("/void").then().statusCode(204).body(hasLength(0));
     }

diff --git a/...ctive-routes/deployment/src/test/java/io/quarkus/vertx/web/mutiny/JsonMultiRouteTest.java b/...ctive-routes/deployment/src/test/java/io/quarkus/vertx/web/mutiny/JsonMultiRouteTest.java
@@ -59,8 +59,8 @@ public void testMultiRoute() {
                 .header("content-type", "application/json;charset=utf-8");
 
         when().get("/failure").then().statusCode(500).body(containsString("boom"));
-        when().get("/null").then().statusCode(500).body(containsString("null"));
-        when().get("/sync-failure").then().statusCode(500).body(containsString("null"));
+        when().get("/null").then().statusCode(500).body(containsString(NullPointerException.class.getName()));
+        when().get("/sync-failure").then().statusCode(500).body(containsString("boom"));
 
     }
 

diff --git a/.../reactive-routes/deployment/src/test/java/io/quarkus/vertx/web/mutiny/MultiRouteTest.java b/.../reactive-routes/deployment/src/test/java/io/quarkus/vertx/web/mutiny/MultiRouteTest.java
@@ -52,8 +52,8 @@ public void testMultiRoute() {
                 .header("content-type", is(nullValue()));
 
         when().get("/failure").then().statusCode(500).body(containsString("boom"));
-        when().get("/null").then().statusCode(500).body(containsString("null"));
-        when().get("/sync-failure").then().statusCode(500).body(containsString("null"));
+        when().get("/null").then().statusCode(500).body(containsString(NullPointerException.class.getName()));
+        when().get("/sync-failure").then().statusCode(500).body(containsString("boom"));
 
     }
 

diff --git a/...ive-routes/deployment/src/test/java/io/quarkus/vertx/web/mutiny/NdjsonMultiRouteTest.java b/...ive-routes/deployment/src/test/java/io/quarkus/vertx/web/mutiny/NdjsonMultiRouteTest.java
@@ -80,8 +80,8 @@ public void testNdjsonMultiRoute() {
                 .header(HttpHeaders.CONTENT_TYPE.toString(), CONTENT_TYPE_STREAM_JSON);
 
         when().get("/failure").then().statusCode(500).body(containsString("boom"));
-        when().get("/null").then().statusCode(500).body(containsString("null"));
-        when().get("/sync-failure").then().statusCode(500).body(containsString("null"));
+        when().get("/null").then().statusCode(500).body(containsString(NullPointerException.class.getName()));
+        when().get("/sync-failure").then().statusCode(500).body(containsString("boom"));
     }
 
     static class SimpleBean {

diff --git a/...active-routes/deployment/src/test/java/io/quarkus/vertx/web/mutiny/SSEMultiRouteTest.java b/...active-routes/deployment/src/test/java/io/quarkus/vertx/web/mutiny/SSEMultiRouteTest.java
@@ -106,8 +106,8 @@ public void testSSEMultiRoute() {
                 .header("content-type", is("text/event-stream"));
 
         when().get("/failure").then().statusCode(500).body(containsString("boom"));
-        when().get("/null").then().statusCode(500).body(containsString("null"));
-        when().get("/sync-failure").then().statusCode(500).body(containsString("null"));
+        when().get("/null").then().statusCode(500).body(containsString(NullPointerException.class.getName()));
+        when().get("/sync-failure").then().statusCode(500).body(containsString("boom"));
 
     }
 

diff --git a/...ns/reactive-routes/deployment/src/test/java/io/quarkus/vertx/web/mutiny/UniRouteTest.java b/...ns/reactive-routes/deployment/src/test/java/io/quarkus/vertx/web/mutiny/UniRouteTest.java
@@ -43,7 +43,7 @@ public void testUniRoute() {
         when().get("/failure").then().statusCode(500).body(containsString("boom"));
         when().get("/sync-failure").then().statusCode(500).body(containsString("boom"));
 
-        when().get("/null").then().statusCode(500).body(containsString("null"));
+        when().get("/null").then().statusCode(500).body(containsString(NullPointerException.class.getName()));
         when().get("/uni-null").then().statusCode(500);
         when().get("/void").then().statusCode(204).body(hasLength(0));
     }

diff --git a/...ions/undertow/deployment/src/test/java/io/quarkus/undertow/test/ErrorServletTestCase.java b/...ions/undertow/deployment/src/test/java/io/quarkus/undertow/test/ErrorServletTestCase.java
@@ -31,7 +31,7 @@ public void testJsonError() {
         RestAssured.given().accept(ContentType.JSON)
                 .when().get("/error").then()
                 .statusCode(500)
-                .body("details", startsWith("Error handling"))
+                .body("details", startsWith("Error id"))
                 .body("stack", startsWith("java.lang.RuntimeException: Error !!!"));
     }
 }
diff --git a/...sions/undertow/runtime/src/main/java/io/quarkus/undertow/runtime/QuarkusErrorServlet.java b/...sions/undertow/runtime/src/main/java/io/quarkus/undertow/runtime/QuarkusErrorServlet.java
@@ -65,7 +65,7 @@ private static String generateStackTrace(final Throwable exception) {
     }
 
     private static String generateHeaderMessage(final Throwable exception, String uuid) {
-        return String.format("Error handling %s, %s: %s", uuid, exception.getClass().getName(),
+        return String.format("Error id %s, %s: %s", uuid, exception.getClass().getName(),
                 extractFirstLine(exception.getMessage()));
     }
 

diff --git a/...ons/vertx-http/deployment/src/test/java/io/quarkus/vertx/http/UnhandledExceptionTest.java b/...ons/vertx-http/deployment/src/test/java/io/quarkus/vertx/http/UnhandledExceptionTest.java
@@ -0,0 +1,264 @@
+package io.quarkus.vertx.http;
+
+import static io.restassured.RestAssured.given;
+import static io.restassured.config.HeaderConfig.headerConfig;
+import static org.hamcrest.Matchers.allOf;
+import static org.hamcrest.Matchers.containsString;
+
+import javax.enterprise.context.ApplicationScoped;
+import javax.enterprise.event.Observes;
+
+import org.hamcrest.Matcher;
+import org.jboss.shrinkwrap.api.ShrinkWrap;
+import org.jboss.shrinkwrap.api.spec.JavaArchive;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+
+import io.quarkus.test.QuarkusUnitTest;
+import io.restassured.RestAssured;
+import io.restassured.config.RestAssuredConfig;
+import io.vertx.core.Handler;
+import io.vertx.ext.web.Router;
+import io.vertx.ext.web.RoutingContext;
+
+public class UnhandledExceptionTest {
+
+    private static final String APPLICATION_JSON = "application/json";
+    private static final String TEXT_JSON = "text/json";
+    private static final String TEXT_HTML = "text/html";
+    private static final String APPLICATION_XML = "application/xml";
+    private static final String TEXT_XML = "text/xml";
+    private static final String APPLICATION_XHTML = "application/xhtml+xml";
+    private static final String APPLICATION_OCTET_STREAM = "application/octet-stream";
+
+    @RegisterExtension
+    static final QuarkusUnitTest config = new QuarkusUnitTest()
+            .setArchiveProducer(() -> ShrinkWrap.create(JavaArchive.class)
+                    .addClasses(BeanRegisteringRouteThatThrowsException.class));
+
+    @Test
+    public void testNoAccept() {
+        given().get("/unhandled-exception")
+                .then()
+                .statusCode(500)
+                .contentType(APPLICATION_JSON) // Default to JSON
+                .body(jsonBodyMatcher());
+    }
+
+    @Test
+    public void testAcceptUnsupported() {
+        given()
+                .accept(APPLICATION_OCTET_STREAM)
+                .get("/unhandled-exception")
+                .then()
+                .statusCode(500)
+                .contentType(APPLICATION_JSON) // Default to JSON
+                .body(jsonBodyMatcher());
+    }
+
+    @Test
+    public void testAcceptApplicationJson() {
+        given()
+                .accept(APPLICATION_JSON)
+                .get("/unhandled-exception")
+                .then()
+                .statusCode(500)
+                .contentType(APPLICATION_JSON)
+                .body(jsonBodyMatcher());
+    }
+
+    @Test
+    public void testAcceptTextJson() {
+        given()
+                .accept(TEXT_JSON)
+                .get("/unhandled-exception")
+                .then()
+                .statusCode(500)
+                .contentType(TEXT_JSON)
+                .body(jsonBodyMatcher());
+    }
+
+    @Test
+    public void testAcceptTextHtml() {
+        given()
+                .accept(TEXT_HTML)
+                .get("/unhandled-exception")
+                .then()
+                .statusCode(500)
+                .contentType(TEXT_HTML)
+                .body(htmlBodyMatcher());
+    }
+
+    @Test
+    public void testAcceptTextXml() {
+        given()
+                .accept(TEXT_XML)
+                .get("/unhandled-exception")
+                .then()
+                .statusCode(500)
+                .contentType(TEXT_HTML) // Not quite what they want, but better than nothing
+                .body(htmlBodyMatcher());
+    }
+
+    @Test
+    public void testAcceptApplicationXml() {
+        given()
+                .accept(APPLICATION_XML)
+                .get("/unhandled-exception")
+                .then()
+                .statusCode(500)
+                .contentType(TEXT_HTML) // Not quite what they want, but better than nothing
+                .body(htmlBodyMatcher());
+    }
+
+    @Test
+    public void testAcceptXHtml() {
+        given()
+                .accept(APPLICATION_XHTML)
+                .get("/unhandled-exception")
+                .then()
+                .statusCode(500)
+                .contentType(TEXT_HTML) // Not quite what they want, but better than nothing
+                .body(htmlBodyMatcher());
+    }
+
+    @Test
+    public void testAcceptWildcard() {
+        given()
+                .accept("text/*")
+                .get("/unhandled-exception")
+                .then()
+                .statusCode(500)
+                .contentType(TEXT_JSON)
+                .body(jsonBodyMatcher());
+    }
+
+    @Test
+    public void testAcceptParameter() {
+        // We don't support accept parameters: they will be ignored.
+        given()
+                .accept("text/html;q=0.8")
+                .get("/unhandled-exception")
+                .then()
+                .statusCode(500)
+                .contentType(TEXT_HTML)
+                .body(htmlBodyMatcher());
+    }
+
+    @Test
+    public void testMultipleAcceptHeaders() {
+        RestAssuredConfig multipleAcceptHeadersConfig = RestAssured.config()
+                .headerConfig(headerConfig().mergeHeadersWithName("Accept"));
+        given()
+                .config(multipleAcceptHeadersConfig)
+                .accept(APPLICATION_JSON)
+                .accept(TEXT_HTML)
+                .accept(APPLICATION_OCTET_STREAM)
+                .get("/unhandled-exception")
+                .then()
+                .statusCode(500)
+                .contentType(APPLICATION_JSON)
+                .body(jsonBodyMatcher());
+
+        given()
+                .config(multipleAcceptHeadersConfig)
+                .accept(TEXT_HTML)
+                .accept(APPLICATION_JSON)
+                .accept(APPLICATION_OCTET_STREAM)
+                .get("/unhandled-exception")
+                .then()
+                .statusCode(500)
+                .contentType(TEXT_HTML)
+                .body(htmlBodyMatcher());
+
+        given()
+                .config(multipleAcceptHeadersConfig)
+                .accept(APPLICATION_OCTET_STREAM)
+                .accept(TEXT_HTML)
+                .accept(APPLICATION_JSON)
+                .get("/unhandled-exception")
+                .then()
+                .statusCode(500)
+                // Ideally we'd like TEXT_HTML here, but due to some strange behavior of
+                // io.vertx.ext.web.ParsedHeaderValues.findBestUserAcceptedIn,
+                // we get this.
+                .contentType(APPLICATION_JSON)
+                .body(jsonBodyMatcher());
+
+        given()
+                .config(multipleAcceptHeadersConfig)
+                .accept(APPLICATION_OCTET_STREAM)
+                .accept(APPLICATION_JSON)
+                .accept(TEXT_HTML)
+                .get("/unhandled-exception")
+                .then()
+                .statusCode(500)
+                .contentType(APPLICATION_JSON)
+                .body(jsonBodyMatcher());
+    }
+
+    @Test
+    public void testCompositeAcceptHeaders() {
+        given()
+                .accept(APPLICATION_JSON + ", " + TEXT_HTML + ", " + APPLICATION_OCTET_STREAM)
+                .get("/unhandled-exception")
+                .then()
+                .statusCode(500)
+                .contentType(APPLICATION_JSON)
+                .body(jsonBodyMatcher());
+
+        given()
+                .accept(TEXT_HTML + ", " + APPLICATION_JSON + ", " + APPLICATION_OCTET_STREAM)
+                .get("/unhandled-exception")
+                .then()
+                .statusCode(500)
+                .contentType(TEXT_HTML)
+                .body(htmlBodyMatcher());
+
+        given()
+                .accept(APPLICATION_OCTET_STREAM + ", " + TEXT_HTML + ", " + APPLICATION_JSON)
+                .get("/unhandled-exception")
+                .then()
+                .statusCode(500)
+                .contentType(TEXT_HTML)
+                .body(htmlBodyMatcher());
+
+        given()
+                .accept(APPLICATION_OCTET_STREAM + ", " + APPLICATION_JSON + ", " + TEXT_HTML)
+                .get("/unhandled-exception")
+                .then()
+                .statusCode(500)
+                .contentType(APPLICATION_JSON)
+                .body(jsonBodyMatcher());
+    }
+
+    private Matcher<String> jsonBodyMatcher() {
+        return allOf(
+                containsString("\"details\":\"Error id"),
+                containsString("\"stack\":\"java.lang.RuntimeException: Simulated failure"),
+                containsString("at " + BeanRegisteringRouteThatThrowsException.class.getName() + "$1.handle"));
+    }
+
+    private Matcher<String> htmlBodyMatcher() {
+        return allOf(
+                containsString("<!doctype html>"),
+                containsString("<title>Internal Server Error"),
+                containsString("java.lang.RuntimeException: Simulated failure"),
+                containsString("at " + BeanRegisteringRouteThatThrowsException.class.getName() + "$1.handle"));
+    }
+
+    @ApplicationScoped
+    static class BeanRegisteringRouteThatThrowsException {
+
+        public void register(@Observes Router router) {
+            router.route("/unhandled-exception").handler(new Handler<RoutingContext>() {
+                @Override
+                public void handle(RoutingContext event) {
+                    throw new RuntimeException("Simulated failure");
+                }
+            });
+        }
+
+    }
+
+}
diff --git a/extensions/vertx-http/runtime/pom.xml b/extensions/vertx-http/runtime/pom.xml
@@ -78,6 +78,16 @@
             <artifactId>jackson-databind</artifactId>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <groupId>org.mockito</groupId>
+            <artifactId>mockito-core</artifactId>
+            <scope>test</scope>
+        </dependency>
+        <dependency>
+            <groupId>org.mockito</groupId>
+            <artifactId>mockito-junit-jupiter</artifactId>
+            <scope>test</scope>
+        </dependency>
     </dependencies>
 
     <build>

diff --git a/...ons/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/HttpConfiguration.java b/...ons/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/HttpConfiguration.java
@@ -231,6 +231,20 @@ public class HttpConfiguration {
     @ConfigItem
     public boolean enableDecompression;
 
+    /**
+     * Provides a hint (optional) for the default content type of responses generated for
+     * the errors not handled by the application.
+     * <p>
+     * If the client requested a supported content-type in request headers
+     * (e.g. "Accept: application/json", "Accept: text/html"),
+     * Quarkus will use that content type.
+     * <p>
+     * Otherwise, it will default to the content type configured here.
+     * </p>
+     */
+    @ConfigItem
+    public Optional<PayloadHint> unhandledErrorContentTypeDefault;
+
     public ProxyConfig proxy;
 
     public int determinePort(LaunchMode launchMode) {
@@ -246,4 +260,9 @@ public enum InsecureRequests {
         REDIRECT,
         DISABLED;
     }
+
+    public enum PayloadHint {
+        JSON,
+        HTML,
+    }
 }