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] 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().