From 5b4b554d1164bdba46ca33adb6e2c1463e65cbf5 Mon Sep 17 00:00:00 2001 From: JCgH4164838Gh792C124B5 <43964333+JCgH4164838Gh792C124B5@users.noreply.github.com> Date: Tue, 17 Sep 2019 22:22:41 -0400 Subject: [PATCH 1/2] Minor improvement proposed for ServletRedirectResult sendRedirect() Supply log warning when an IOException or IllegalStateException occurs to better allow developers to track the failed redirect location and status details. The exceptions are re-thrown to ensure existing flow-control behaviour is preserved. When getWriter() is called, utilize a finally block to ensure close is called. --- .../struts2/result/ServletRedirectResult.java | 25 +++++++++++++------ 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/result/ServletRedirectResult.java b/core/src/main/java/org/apache/struts2/result/ServletRedirectResult.java index 0879ab06d..704f80ab6 100644 --- a/core/src/main/java/org/apache/struts2/result/ServletRedirectResult.java +++ b/core/src/main/java/org/apache/struts2/result/ServletRedirectResult.java @@ -250,13 +250,24 @@ public class ServletRedirectResult extends StrutsResultSupport implements Reflec * @throws IOException in case of IO errors */ protected void sendRedirect(HttpServletResponse response, String finalLocation) throws IOException { - if (SC_FOUND == statusCode) { - response.sendRedirect(finalLocation); - } else { - response.setStatus(statusCode); - response.setHeader("Location", finalLocation); - response.getWriter().write(finalLocation); - response.getWriter().close(); + try { + if (SC_FOUND == statusCode) { + response.sendRedirect(finalLocation); + } else { + response.setStatus(statusCode); + response.setHeader("Location", finalLocation); + try { + response.getWriter().write(finalLocation); + } finally { + response.getWriter().close(); + } + } + } catch (IOException ioe) { + LOG.warn("Unable to redirect to: {}, code: {}! (IOException): {}", finalLocation, statusCode, ioe.toString()); + throw ioe; // Re-throw required to preserve existing default behaviour + } catch (IllegalStateException ise) { + LOG.warn("Unable to redirect to: {}, code: {}! isCommited: {}. (IllegalStateException): {}", finalLocation, statusCode, response.isCommitted(), ise.toString()); + throw ise; // Re-throw required to preserve existing default behaviour } } From d88a8382d412d4c355cbfa88de1a44b3f4d4bc00 Mon Sep 17 00:00:00 2001 From: JCgH4164838Gh792C124B5 <43964333+JCgH4164838Gh792C124B5@users.noreply.github.com> Date: Sun, 22 Sep 2019 21:45:44 -0400 Subject: [PATCH 2/2] Changed the styling of log outputs (as suggested by A. Mashchenko and L. Lenart). Added comment to explain why the log output avoids a stacktrace (due to exceptions being re-thrown). Updated existing ServletRedirectResultTest to supply tests that exercise the new code paths and demonstrate expected logging. --- .../struts2/result/ServletRedirectResult.java | 8 +- .../result/ServletRedirectResultTest.java | 119 ++++++++++++++++++ 2 files changed, 123 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/result/ServletRedirectResult.java b/core/src/main/java/org/apache/struts2/result/ServletRedirectResult.java index 704f80ab6..defb5b589 100644 --- a/core/src/main/java/org/apache/struts2/result/ServletRedirectResult.java +++ b/core/src/main/java/org/apache/struts2/result/ServletRedirectResult.java @@ -263,11 +263,11 @@ public class ServletRedirectResult extends StrutsResultSupport implements Reflec } } } catch (IOException ioe) { - LOG.warn("Unable to redirect to: {}, code: {}! (IOException): {}", finalLocation, statusCode, ioe.toString()); - throw ioe; // Re-throw required to preserve existing default behaviour + LOG.warn("Unable to redirect to: {}, code: {}; {}", finalLocation, statusCode, ioe); + throw ioe; // Re-throw required to preserve existing default behaviour (no stacktrace in above warn for this reason) } catch (IllegalStateException ise) { - LOG.warn("Unable to redirect to: {}, code: {}! isCommited: {}. (IllegalStateException): {}", finalLocation, statusCode, response.isCommitted(), ise.toString()); - throw ise; // Re-throw required to preserve existing default behaviour + LOG.warn("Unable to redirect to: {}, code: {}; isCommited: {}; {}", finalLocation, statusCode, response.isCommitted(), ise); + throw ise; // Re-throw required to preserve existing default behaviour (no stacktrace in above warn for this reason) } } diff --git a/core/src/test/java/org/apache/struts2/result/ServletRedirectResultTest.java b/core/src/test/java/org/apache/struts2/result/ServletRedirectResultTest.java index b17907f04..007b50852 100644 --- a/core/src/test/java/org/apache/struts2/result/ServletRedirectResultTest.java +++ b/core/src/test/java/org/apache/struts2/result/ServletRedirectResultTest.java @@ -21,9 +21,12 @@ package org.apache.struts2.result; import static javax.servlet.http.HttpServletResponse.SC_SEE_OTHER; import static org.easymock.EasyMock.createControl; import static org.easymock.EasyMock.createNiceMock; +import static org.easymock.EasyMock.createMock; import static org.easymock.EasyMock.expect; +import static org.easymock.EasyMock.expectLastCall; import static org.easymock.EasyMock.replay; +import java.io.IOException; import java.io.PrintWriter; import java.io.StringWriter; import java.util.ArrayList; @@ -321,6 +324,122 @@ public class ServletRedirectResultTest extends StrutsInternalTestCase implements control.verify(); } + /** + * Test to exercise the code path and prove sendRedirect() will output + * the desired log warning when an IOException is thrown for statusCode SC_FOUND. + */ + public void testSendRedirectSCFoundIOException() { + HttpServletResponse httpServletResponseMock = (HttpServletResponse) createMock(HttpServletResponse.class); + boolean ioeCaught = false; + view.setLocation("/bar/foo.jsp"); + view.setStatusCode(HttpServletResponse.SC_FOUND); + try { + httpServletResponseMock.sendRedirect(view.getLocation()); + expectLastCall().andStubThrow(new IOException("Fake IO Exception (SC_FOUND)")); + replay(httpServletResponseMock); + } catch (IOException ioe) { + fail("Mock sendRedirect call setup failed. Ex: " + ioe); + } + try { + view.sendRedirect(httpServletResponseMock, view.getLocation()); + } catch (IOException ioe) { + ioeCaught = true; // Verify expected exception was thrown + } + if (!ioeCaught) { + fail("sendRedirect (SC_FOUND) with forced IOException did not propagate from setLocation!"); + } + } + + /** + * Test to exercise the code path and prove sendRedirect() will output + * the desired log warning when an IOException is thrown for statusCode SC_MOVED_PERMANENTLY. + */ + public void testSendRedirectSCMovedPermanentlyIOException() { + HttpServletResponse httpServletResponseMock = (HttpServletResponse) createMock(HttpServletResponse.class); + boolean ioeCaught = false; + view.setLocation("/bar/foo.jsp"); + view.setStatusCode(HttpServletResponse.SC_MOVED_PERMANENTLY); // Any non SC_FOUND will suffice + try { + httpServletResponseMock.setStatus(HttpServletResponse.SC_MOVED_PERMANENTLY); + expectLastCall(); + httpServletResponseMock.setHeader("Location", view.getLocation()); + expectLastCall(); + expect(httpServletResponseMock.getWriter()).andStubThrow(new IOException("Fake IO Exception (SC_MOVED_PERMANENTLY)")); + replay(httpServletResponseMock); + } catch (IOException ioe) { + fail("Mock getWriter call setup failed. Ex: " + ioe); + } + try { + view.sendRedirect(httpServletResponseMock, view.getLocation()); + } catch (IOException ioe) { + ioeCaught = true; // Verify expected exception was thrown + } + if (!ioeCaught) { + fail("sendRedirect (SC_MOVED_PERMANENTLY) with forced IOException did not propagate from setLocation!"); + } + } + + /** + * Test to exercise the code path and prove sendRedirect() will output + * the desired log warning when an IllegalStateException is thrown for statusCode SC_FOUND. + */ + public void testSendRedirectSCFoundIllegalStateException() { + HttpServletResponse httpServletResponseMock = (HttpServletResponse) createMock(HttpServletResponse.class); + boolean iseCaught = false; + view.setLocation("/bar/foo.jsp"); + view.setStatusCode(HttpServletResponse.SC_FOUND); + try { + httpServletResponseMock.sendRedirect(view.getLocation()); + expectLastCall().andStubThrow(new IllegalStateException("Fake IllegalState Exception (SC_FOUND)")); + expect(httpServletResponseMock.isCommitted()).andStubReturn(Boolean.TRUE); + replay(httpServletResponseMock); + } catch (IOException ioe) { + fail("Mock sendRedirect call setup failed. Ex: " + ioe); + } + try { + view.sendRedirect(httpServletResponseMock, view.getLocation()); + } catch (IOException ioe) { + iseCaught = false; + } catch (IllegalStateException ise) { + iseCaught = true; // Verify expected exception was thrown + } + if (!iseCaught) { + fail("sendRedirect (SC_FOUND) with forced IllegalStateException did not propagate from setLocation!"); + } + } + + /** + * Test to exercise the code path and prove sendRedirect() will output + * the desired log warning when an IllegalStateException is thrown for statusCode SC_MOVED_PERMANENTLY. + */ + public void testSendRedirectSCMovedPermanentlyIllegalStateException() { + HttpServletResponse httpServletResponseMock = (HttpServletResponse) createMock(HttpServletResponse.class); + boolean iseCaught = false; + view.setLocation("/bar/foo.jsp"); + view.setStatusCode(HttpServletResponse.SC_MOVED_PERMANENTLY); // Any non SC_FOUND will suffice + try { + httpServletResponseMock.setStatus(HttpServletResponse.SC_MOVED_PERMANENTLY); + expectLastCall(); + httpServletResponseMock.setHeader("Location", view.getLocation()); + expectLastCall(); + expect(httpServletResponseMock.getWriter()).andStubThrow(new IllegalStateException("Fake IllegalState Exception (SC_MOVED_PERMANENTLY)")); + expect(httpServletResponseMock.isCommitted()).andStubReturn(Boolean.TRUE); + replay(httpServletResponseMock); + } catch (IOException ioe) { + fail("Mock getWriter call setup failed. Ex: " + ioe); + } + try { + view.sendRedirect(httpServletResponseMock, view.getLocation()); + } catch (IOException ioe) { + iseCaught = false; + } catch (IllegalStateException ise) { + iseCaught = true; // Verify expected exception was thrown + } + if (!iseCaught) { + fail("sendRedirect (SC_MOVED_PERMANENTLY) with forced IllegalStateException did not propagate from setLocation!"); + } + } + protected void setUp() throws Exception { super.setUp(); configurationManager.getConfiguration().