From a06de4d997103b377af349b392626cb2c18c31bf Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Mon, 9 Apr 2018 16:14:27 +0100 Subject: [PATCH] Stop error page filter from commiting response prematurely Previously, the error page filter used sendError to set the response status when handling an exception and before forwarding the request to the error controller. Following the fix for gh-11814, this meant that the error controller was unable to write its response and the containers default error page was returned instead. This commit updates the error page filter to use setStatus rather than sendError. This ensures that the response has the correct status code while allowing the error controller to write its body. Tests have been added to the Tomcat deployment test suite to verify that the error page filter behaves as intended when dealing with a sent error and an exception for requests accepting HTML, JSON, or anything. Closes gh-12787 --- .../main/java/sample/SampleController.java | 16 +++- .../SampleTomcatDeployApplicationIT.java | 78 ++++++++++++++++++- .../boot/web/support/ErrorPageFilter.java | 2 +- 3 files changed, 91 insertions(+), 5 deletions(-) diff --git a/spring-boot-deployment-tests/spring-boot-deployment-test-tomcat/src/main/java/sample/SampleController.java b/spring-boot-deployment-tests/spring-boot-deployment-test-tomcat/src/main/java/sample/SampleController.java index bdee6478b1..c1e59b089f 100644 --- a/spring-boot-deployment-tests/spring-boot-deployment-test-tomcat/src/main/java/sample/SampleController.java +++ b/spring-boot-deployment-tests/spring-boot-deployment-test-tomcat/src/main/java/sample/SampleController.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2016 the original author or authors. + * Copyright 2012-2018 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -16,6 +16,10 @@ package sample; +import java.io.IOException; + +import javax.servlet.http.HttpServletResponse; + import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RestController; @@ -27,4 +31,14 @@ public class SampleController { return "Hello World"; } + @RequestMapping("/send-error") + public void sendError(HttpServletResponse response) throws IOException { + response.sendError(500); + } + + @RequestMapping("/exception") + public void exception() { + throw new RuntimeException(); + } + } diff --git a/spring-boot-deployment-tests/spring-boot-deployment-test-tomcat/src/test/java/sample/SampleTomcatDeployApplicationIT.java b/spring-boot-deployment-tests/spring-boot-deployment-test-tomcat/src/test/java/sample/SampleTomcatDeployApplicationIT.java index 0a54c7c206..a592caeaee 100644 --- a/spring-boot-deployment-tests/spring-boot-deployment-test-tomcat/src/test/java/sample/SampleTomcatDeployApplicationIT.java +++ b/spring-boot-deployment-tests/spring-boot-deployment-test-tomcat/src/test/java/sample/SampleTomcatDeployApplicationIT.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2016 the original author or authors. + * Copyright 2012-2018 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -16,10 +16,14 @@ package sample; +import java.net.URI; + import org.junit.Test; import org.springframework.boot.test.web.client.TestRestTemplate; import org.springframework.http.HttpStatus; +import org.springframework.http.MediaType; +import org.springframework.http.RequestEntity; import org.springframework.http.ResponseEntity; import static org.assertj.core.api.Assertions.assertThat; @@ -29,15 +33,83 @@ import static org.assertj.core.api.Assertions.assertThat; */ public class SampleTomcatDeployApplicationIT { + private final TestRestTemplate rest = new TestRestTemplate(); + private int port = Integer.valueOf(System.getProperty("port")); @Test public void testHome() throws Exception { String url = "http://localhost:" + this.port + "/bootapp/"; - ResponseEntity entity = new TestRestTemplate().getForEntity(url, - String.class); + ResponseEntity entity = this.rest.getForEntity(url, String.class); assertThat(entity.getStatusCode()).isEqualTo(HttpStatus.OK); assertThat(entity.getBody()).isEqualTo("Hello World"); } + @Test + public void errorFromExceptionForRequestAcceptingAnythingProducesAJsonResponse() + throws Exception { + assertThatErrorFromExceptionProducesExpectedResponse(MediaType.ALL, + MediaType.APPLICATION_JSON); + } + + @Test + public void errorFromExceptionForRequestAcceptingJsonProducesAJsonResponse() + throws Exception { + assertThatErrorFromExceptionProducesExpectedResponse(MediaType.APPLICATION_JSON, + MediaType.APPLICATION_JSON); + } + + @Test + public void errorFromExceptionForRequestAcceptingHtmlProducesAnHtmlResponse() + throws Exception { + assertThatErrorFromExceptionProducesExpectedResponse(MediaType.TEXT_HTML, + MediaType.TEXT_HTML); + } + + @Test + public void sendErrorForRequestAcceptingAnythingProducesAJsonResponse() + throws Exception { + assertThatSendErrorProducesExpectedResponse(MediaType.ALL, + MediaType.APPLICATION_JSON); + } + + @Test + public void sendErrorForRequestAcceptingJsonProducesAJsonResponse() throws Exception { + assertThatSendErrorProducesExpectedResponse(MediaType.APPLICATION_JSON, + MediaType.APPLICATION_JSON); + } + + @Test + public void sendErrorForRequestAcceptingHtmlProducesAnHtmlResponse() + throws Exception { + assertThatSendErrorProducesExpectedResponse(MediaType.TEXT_HTML, + MediaType.TEXT_HTML); + } + + private void assertThatSendErrorProducesExpectedResponse(MediaType accept, + MediaType contentType) { + RequestEntity request = RequestEntity + .get(URI.create("http://localhost:" + this.port + "/bootapp/send-error")) + .accept(accept).build(); + ResponseEntity response = this.rest.exchange(request, String.class); + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.INTERNAL_SERVER_ERROR); + assertThat(contentType.isCompatibleWith(response.getHeaders().getContentType())) + .as("%s is compatible with %s", contentType, + response.getHeaders().getContentType()) + .isTrue(); + } + + private void assertThatErrorFromExceptionProducesExpectedResponse(MediaType accept, + MediaType contentType) { + RequestEntity request = RequestEntity + .get(URI.create("http://localhost:" + this.port + "/bootapp/exception")) + .accept(accept).build(); + ResponseEntity response = this.rest.exchange(request, String.class); + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.INTERNAL_SERVER_ERROR); + assertThat(contentType.isCompatibleWith(response.getHeaders().getContentType())) + .as("%s is compatible with %s", contentType, + response.getHeaders().getContentType()) + .isTrue(); + } + } diff --git a/spring-boot/src/main/java/org/springframework/boot/web/support/ErrorPageFilter.java b/spring-boot/src/main/java/org/springframework/boot/web/support/ErrorPageFilter.java index 4466391401..b1f58789c3 100644 --- a/spring-boot/src/main/java/org/springframework/boot/web/support/ErrorPageFilter.java +++ b/spring-boot/src/main/java/org/springframework/boot/web/support/ErrorPageFilter.java @@ -181,7 +181,7 @@ public class ErrorPageFilter implements Filter, ErrorPageRegistry { request.setAttribute(ERROR_EXCEPTION, ex); request.setAttribute(ERROR_EXCEPTION_TYPE, ex.getClass()); response.reset(); - response.sendError(500, ex.getMessage()); + response.setStatus(500); request.getRequestDispatcher(path).forward(request, response); request.removeAttribute(ERROR_EXCEPTION); request.removeAttribute(ERROR_EXCEPTION_TYPE);