Align servlet container error handling with executable jar/war behavior

Previously, when an exception was thrown by a Controller in an
application deployed to a servlet container the exception that was
handled would be Spring Framework’s NestedServletException rather than
the exception thrown by the application. Furthermore, when an exception
was thrown or the response was used to send an error, the
javax.servlet.error.request_uri request attribute would not be set. This
differed from the behaviour in an executable jar/war where the exception
would be the one thrown by the application, and the request_uri
attribute would be set.

This commit updates ErrorPageFilter, which is only involved in a servlet
container, to unwrap a NestedServletException so that it’s the
application’s exception that’s handled, and to set the request_uri
attribute in the event of an exception being thrown or an error being
sent.

Closes gh-3249

Author: wilkinsona

spring-boot/src/main/java/org/springframework/boot/context/web/ErrorPageFilter.java

@@ -39,6 +39,7 @@
 import org.springframework.core.annotation.Order;
 import org.springframework.stereotype.Component;
 import org.springframework.web.filter.OncePerRequestFilter;
+import org.springframework.web.util.NestedServletException;
 
 /**
  * A special {@link AbstractConfigurableEmbeddedServletContainer} for non-embedded
@@ -69,6 +70,8 @@ public class ErrorPageFilter extends AbstractConfigurableEmbeddedServletContaine
 
 	private static final String ERROR_MESSAGE = "javax.servlet.error.message";
 
+	public static final String ERROR_REQUEST_URI = "javax.servlet.error.request_uri";
+
 	private static final String ERROR_STATUS_CODE = "javax.servlet.error.status_code";
 
 	private String global;
@@ -121,7 +124,11 @@ else if (!request.isAsyncStarted() && !response.isCommitted()) {
 			}
 		}
 		catch (Throwable ex) {
-			handleException(request, response, wrapped, ex);
+			Throwable exceptionToHandle = ex;
+			if (ex instanceof NestedServletException) {
+				exceptionToHandle = ((NestedServletException) ex).getRootCause();
+			}
+			handleException(request, response, wrapped, exceptionToHandle);
 			response.flushBuffer();
 		}
 	}
@@ -225,9 +232,10 @@ private String getErrorPath(Class<?> type) {
 		return this.global;
 	}
 
-	private void setErrorAttributes(ServletRequest request, int status, String message) {
+	private void setErrorAttributes(HttpServletRequest request, int status, String message) {
 		request.setAttribute(ERROR_STATUS_CODE, status);
 		request.setAttribute(ERROR_MESSAGE, message);
+		request.setAttribute(ERROR_REQUEST_URI, request.getRequestURI());
 	}
 
 	private void rethrow(Throwable ex) throws IOException, ServletException {

spring-boot/src/test/java/org/springframework/boot/context/web/ErrorPageFilterTests.java

@@ -38,6 +38,7 @@
 import org.springframework.web.context.request.async.StandardServletAsyncWebRequest;
 import org.springframework.web.context.request.async.WebAsyncManager;
 import org.springframework.web.context.request.async.WebAsyncUtils;
+import org.springframework.web.util.NestedServletException;
 
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
@@ -62,7 +63,8 @@ public class ErrorPageFilterTests {
 
 	private ErrorPageFilter filter = new ErrorPageFilter();
 
-	private MockHttpServletRequest request = new MockHttpServletRequest();
+	private MockHttpServletRequest request = new MockHttpServletRequest("GET",
+			"/test/path");
 
 	private MockHttpServletResponse response = new MockHttpServletResponse();
 
@@ -199,6 +201,9 @@ public void doFilter(ServletRequest request, ServletResponse response)
 				equalTo((Object) 400));
 		assertThat(this.request.getAttribute(RequestDispatcher.ERROR_MESSAGE),
 				equalTo((Object) "BAD"));
+		assertThat(this.request.getAttribute(RequestDispatcher.ERROR_REQUEST_URI),
+				equalTo((Object) "/test/path"));
+
 		assertTrue(this.response.isCommitted());
 		assertThat(this.response.getForwardedUrl(), equalTo("/error"));
 	}
@@ -221,6 +226,8 @@ public void doFilter(ServletRequest request, ServletResponse response)
 				equalTo((Object) 400));
 		assertThat(this.request.getAttribute(RequestDispatcher.ERROR_MESSAGE),
 				equalTo((Object) "BAD"));
+		assertThat(this.request.getAttribute(RequestDispatcher.ERROR_REQUEST_URI),
+				equalTo((Object) "/test/path"));
 		assertTrue(this.response.isCommitted());
 		assertThat(this.response.getForwardedUrl(), equalTo("/400"));
 	}
@@ -264,6 +271,8 @@ public void doFilter(ServletRequest request, ServletResponse response)
 				equalTo((Object) "BAD"));
 		assertThat(this.request.getAttribute(RequestDispatcher.ERROR_EXCEPTION_TYPE),
 				equalTo((Object) RuntimeException.class.getName()));
+		assertThat(this.request.getAttribute(RequestDispatcher.ERROR_REQUEST_URI),
+				equalTo((Object) "/test/path"));
 		assertTrue(this.response.isCommitted());
 		assertThat(this.response.getForwardedUrl(), equalTo("/500"));
 	}
@@ -319,6 +328,8 @@ public void doFilter(ServletRequest request, ServletResponse response)
 				equalTo((Object) "BAD"));
 		assertThat(this.request.getAttribute(RequestDispatcher.ERROR_EXCEPTION_TYPE),
 				equalTo((Object) IllegalStateException.class.getName()));
+		assertThat(this.request.getAttribute(RequestDispatcher.ERROR_REQUEST_URI),
+				equalTo((Object) "/test/path"));
 		assertTrue(this.response.isCommitted());
 	}
 
@@ -465,6 +476,32 @@ public void doFilter(ServletRequest request, ServletResponse response)
 		assertThat(this.output.toString(), containsString("request [/test/alpha]"));
 	}
 
+	@Test
+	public void nestedServletExceptionIsUnwrapped() throws Exception {
+		this.filter.addErrorPages(new ErrorPage(RuntimeException.class, "/500"));
+		this.chain = new MockFilterChain() {
+			@Override
+			public void doFilter(ServletRequest request, ServletResponse response)
+					throws IOException, ServletException {
+				super.doFilter(request, response);
+				throw new NestedServletException("Wrapper", new RuntimeException("BAD"));
+			}
+		};
+		this.filter.doFilter(this.request, this.response, this.chain);
+		assertThat(((HttpServletResponseWrapper) this.chain.getResponse()).getStatus(),
+				equalTo(500));
+		assertThat(this.request.getAttribute(RequestDispatcher.ERROR_STATUS_CODE),
+				equalTo((Object) 500));
+		assertThat(this.request.getAttribute(RequestDispatcher.ERROR_MESSAGE),
+				equalTo((Object) "BAD"));
+		assertThat(this.request.getAttribute(RequestDispatcher.ERROR_EXCEPTION_TYPE),
+				equalTo((Object) RuntimeException.class.getName()));
+		assertThat(this.request.getAttribute(RequestDispatcher.ERROR_REQUEST_URI),
+				equalTo((Object) "/test/path"));
+		assertTrue(this.response.isCommitted());
+		assertThat(this.response.getForwardedUrl(), equalTo("/500"));
+	}
+
 	private void setUpAsyncDispatch() throws Exception {
 		this.request.setAsyncSupported(true);
 		this.request.setAsyncStarted(true);