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
This commit is contained in:
parent
3634a1d9d1
commit
a06de4d997
|
|
@ -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();
|
||||
}
|
||||
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<String> entity = new TestRestTemplate().getForEntity(url,
|
||||
String.class);
|
||||
ResponseEntity<String> 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<Void> request = RequestEntity
|
||||
.get(URI.create("http://localhost:" + this.port + "/bootapp/send-error"))
|
||||
.accept(accept).build();
|
||||
ResponseEntity<String> 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<Void> request = RequestEntity
|
||||
.get(URI.create("http://localhost:" + this.port + "/bootapp/exception"))
|
||||
.accept(accept).build();
|
||||
ResponseEntity<String> 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();
|
||||
}
|
||||
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
Loading…
Reference in New Issue